Обсуждение: Another incorrect comment for pg_stat_statements

Hi, hackers

There has $subject that introduced by commit 6b4d23feef6.  When we reset the entries
if all parameters are avaiable, non-top-level entries removed first, then top-level
entries.

On branch REL_15_STABLE
Your branch is up to date with 'origin/REL_15_STABLE'.

Changes not staged for commit:
  (use "git add <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
    modified:   contrib/pg_stat_statements/pg_stat_statements.c

no changes added to commit (use "git add" and/or "git commit -a")

-- 
Regrads,
Japin Li.

On Wed, 28 Jun 2023 at 11:22, Richard Guo <guofenglinux@gmail.com> wrote:
> On Wed, Jun 28, 2023 at 10:53 AM Japin Li <japinli@hotmail.com> wrote:
>
>>
>> Hi, hackers
>>
>> There has $subject that introduced by commit 6b4d23feef6.  When we reset
>> the entries
>> if all parameters are avaiable, non-top-level entries removed first, then
>> top-level
>> entries.
>
>
> I did not see the diffs.  Maybe uploaded the wrong attachment?
>

My bad!  Here is the patch.  Thanks!

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 21bede29fe..670c993816 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2552,7 +2552,7 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid)
         key.dbid = dbid;
         key.queryid = queryid;
 
-        /* Remove the key if it exists, starting with the top-level entry  */
+        /* Remove the key if it exists, starting with the non-top-level entry */
         key.toplevel = false;
         entry = (pgssEntry *) hash_search(pgss_hash, &key, HASH_REMOVE, NULL);
         if (entry)                /* found */


-- 
Regrads,
Japin Li.

On Wed, Jun 28, 2023 at 12:15:47PM +0800, Japin Li wrote:
> -        /* Remove the key if it exists, starting with the top-level entry  */
> +        /* Remove the key if it exists, starting with the non-top-level entry */
>          key.toplevel = false;
>          entry = (pgssEntry *) hash_search(pgss_hash, &key, HASH_REMOVE, NULL);
>          if (entry)                /* found */

Nice catch.  That's indeed wrong.  Will fix.
--
Michael

On Wed, Jun 28, 2023 at 03:09:55PM +0800, Richard Guo wrote:
> +1.  To nitpick, how about we remove the blank line just before removing
> the key for top level entry?
>
> -  /* Also remove entries for top level statements */
> +  /* Also remove entries if exist for top level statements */
>    key.toplevel = true;
> -
> -  /* Remove the key if exists */
>    entry = (pgssEntry *) hash_search(pgss_hash, &key, HASH_REMOVE, NULL);

Why not if it improves the overall situation.  Could you send a patch
with everything you have in mind?
--
Michael

On Wed, 28 Jun 2023 at 16:27, Richard Guo <guofenglinux@gmail.com> wrote:
> On Wed, Jun 28, 2023 at 3:36 PM Michael Paquier <michael@paquier.xyz> wrote:
>
>> On Wed, Jun 28, 2023 at 03:09:55PM +0800, Richard Guo wrote:
>> > +1.  To nitpick, how about we remove the blank line just before removing
>> > the key for top level entry?
>> >
>> > -  /* Also remove entries for top level statements */
>> > +  /* Also remove entries if exist for top level statements */
>> >    key.toplevel = true;
>> > -
>> > -  /* Remove the key if exists */
>> >    entry = (pgssEntry *) hash_search(pgss_hash, &key, HASH_REMOVE, NULL);
>>
>> Why not if it improves the overall situation.  Could you send a patch
>> with everything you have in mind?
>
>
> Here is the patch.  I don't have too much in mind, so the patch just
> removes the blank line and revises the comment a bit.
>

+1. LGTM.

--
Regrads,
Japin Li.

On Wed, Jun 28, 2023 at 09:26:02PM +0800, Japin Li wrote:
> +1. LGTM.

Nothing much to add, so applied with the initial comment fix.
--
Michael

On Thu, 29 Jun 2023 at 08:19, Michael Paquier <michael@paquier.xyz> wrote:
> On Wed, Jun 28, 2023 at 09:26:02PM +0800, Japin Li wrote:
>> +1. LGTM.
>
> Nothing much to add, so applied with the initial comment fix.

Thanks!

-- 
Regrads,
Japin Li.