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

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!

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

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, 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

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.

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