On Wed, Nov 14, 2018 at 7:04 AM Haribabu Kommi <kommi.harib...@gmail.com> wrote: > > On Wed, Nov 14, 2018 at 12:26 AM Amit Kapila <amit.kapil...@gmail.com> wrote: >> >> On Tue, Nov 13, 2018 at 11:32 AM Haribabu Kommi >> <kommi.harib...@gmail.com> wrote: >> > >> > On Mon, Nov 12, 2018 at 6:34 PM Amit Kapila <amit.kapil...@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:
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))
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.
I have fixed above two observations in the attached patch and edited
few comments and doc changes. Kindly review the same.
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.
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
0001-pg_stat_statements_reset-to-reset-specific-query-use_v10.patch
Description: Binary data
