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