Another incorrect comment for pg_stat_statements

Started by Japin Liover 2 years ago11 messages

japinli@hotmail.com

over 2 years ago

1 attachment(s)

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.

Attachments:

fix-comment-for-entry_reset.patchtext/x-diffDownload

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")

Richard Guo

guofenglinux@gmail.com

over 2 years ago

In reply to: Japin Li (#1)

Re: Another incorrect comment for pg_stat_statements

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?

Thanks
Richard

Japin Li

japinli@hotmail.com

over 2 years ago

In reply to: Richard Guo (#2)

1 attachment(s)

Re: Another incorrect comment for pg_stat_statements

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!

Attachments:

fix-comment-for-entry_reset.patchtext/x-diffDownload

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 */

Michael Paquier

michael@paquier.xyz

over 2 years ago

In reply to: Japin Li (#3)

Re: Another incorrect comment for pg_stat_statements

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

Richard Guo

guofenglinux@gmail.com

over 2 years ago

In reply to: Michael Paquier (#4)

Re: Another incorrect comment for pg_stat_statements

On Wed, Jun 28, 2023 at 3:04 PM Michael Paquier <michael@paquier.xyz> wrote:

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.

+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);

Thanks
Richard

Michael Paquier

michael@paquier.xyz

over 2 years ago

In reply to: Richard Guo (#5)

Re: Another incorrect comment for pg_stat_statements

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

Richard Guo

guofenglinux@gmail.com

over 2 years ago

In reply to: Michael Paquier (#6)

1 attachment(s)

Re: Another incorrect comment for pg_stat_statements

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.

Thanks
Richard

Attachments:

v1-0001-trivial-revise-for-entry_reset.patchapplication/octet-stream; name=v1-0001-trivial-revise-for-entry_reset.patchDownload

From 2d5af4c51a5009134e6a75c910b2c555de94e1c6 Mon Sep 17 00:00:00 2001
From: Richard Guo <guofenglinux@gmail.com>
Date: Wed, 28 Jun 2023 16:18:37 +0800
Subject: [PATCH v1] trivial revise for entry_reset

---
 contrib/pg_stat_statements/pg_stat_statements.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 8dcb2ddd64..1b4103e0b9 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -2552,10 +2552,8 @@ entry_reset(Oid userid, Oid dbid, uint64 queryid)
 		if (entry)				/* found */
 			num_remove++;
 
-		/* Also remove entries for top level statements */
+		/* Also remove the top-level entry if it exists. */
 		key.toplevel = true;
-
-		/* Remove the key if exists */
 		entry = (pgssEntry *) hash_search(pgss_hash, &key, HASH_REMOVE, NULL);
 		if (entry)				/* found */
 			num_remove++;
-- 
2.31.0

Japin Li

japinli@hotmail.com

over 2 years ago

In reply to: Richard Guo (#7)

Re: Another incorrect comment for pg_stat_statements

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.

Michael Paquier

michael@paquier.xyz

over 2 years ago

In reply to: Japin Li (#8)

Re: Another incorrect comment for pg_stat_statements

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

#10

Japin Li

japinli@hotmail.com

over 2 years ago

In reply to: Michael Paquier (#9)

Re: Another incorrect comment for pg_stat_statements

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.

#11

Richard Guo

guofenglinux@gmail.com

over 2 years ago

In reply to: Michael Paquier (#9)

Re: Another incorrect comment for pg_stat_statements

On Thu, Jun 29, 2023 at 8:19 AM Michael Paquier <michael@paquier.xyz> wrote:

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

Thanks for pushing it!

Thanks
Richard