Re: New function pg_stat_statements_reset_query() to reset statistics of a specific query

On Wed, Nov 14, 2018 at 8:45 PM Amit Kapila <amit.kapila16@gmail.com> wrote:

> On Wed, Nov 14, 2018 at 7:04 AM Haribabu Kommi <kommi.haribabu@gmail.com>
> wrote:
> >
> > On Wed, Nov 14, 2018 at 12:26 AM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >>
> >> On Tue, Nov 13, 2018 at 11:32 AM Haribabu Kommi
> >> <kommi.haribabu@gmail.com> wrote:
> >> >
> >> > On Mon, Nov 12, 2018 at 6:34 PM Amit Kapila <amit.kapila16@gmail.com>
> wrote:
> >> >> > I can revert it back to void,
> >> >> >
> >> >>
> >> >> +1, as we don't see any good reason to break backward compatibility.
> >> >
> >> >
> >> > Thanks for the review.
> >> > Attached the updated patch with return type as void.
> >> >
> >>
> >> With this patch, we are intending to remove the entries matching
> >> userid, dbid, queryid from hash table (pgss_hash), but the contents of
> >> the file (
> >> pgss_query_texts.stat) will remain unchanged unless all of the entries
> >> are removed from hash table.  This appears fine to me, especially
> >> because there is no easy way to remove the contents from the file.
> >> Does anybody see any problem with this behavior?
> >
> >
> > Adding more info to the above point, even if the file contents are not
> > removed, later if the file size and number of pg_stat_statements entries
> > satisfy the garbage collection, the file will be truncated. So I feel not
> > removing of the contents when the query stats are reset using specific
> > parameters is fine.
> >
>
> I have further reviewed this patch and below are my comments:
>

Thanks for the review.


> 1.
> + if ((!userid || (userid && entry->key.userid == userid)) &&
> + (!dbid || (dbid && entry->key.dbid == dbid)) &&
> + (!queryid || (queryid && entry->key.queryid == queryid)))
>
> I think this check can be simplified as:
> + if ((!userid || entry->key.userid == userid) &&
> + (!dbid || entry->key.dbid == dbid) &&
> + (!queryid || entry->key.queryid == queryid))
>

Yes, the second check is not necessary.


> 2.
> + else
> + {
> + hash_seq_init(&hash_seq, pgss_hash);
> + while ((entry = hash_seq_search(&hash_seq)) != NULL)
> + {
> + if ((!userid || (userid && entry->key.userid == userid)) &&
> + (!dbid || (dbid && entry->key.dbid == dbid)) &&
> + (!queryid || (queryid && entry->key.queryid == queryid)))
> + {
> + hash_search(pgss_hash, &entry->key, HASH_REMOVE, NULL);
> + num_remove++;
> + }
> + }
> + }
>
> I think this 'if check' is redundant when none of the parameters are
> specified.  We can easily avoid it, see attached.
>

Yes, that removes the unnecessary if check if none of the parameters
are available.

I have fixed above two observations in the attached patch and edited
> few comments and doc changes.  Kindly review the same.
>

Thanks for the correction, all are fine.


> Apart from the above, I think we should add a test where all the
> parameters are valid as the corresponding code is not covered by any
> existing tests.
>

Added another test with all the 3 parameters are valid.
Updated patch attached.

Regards,
Haribabu Kommi
Fujitsu Australia