2020-11-17 01:46 に Fujii Masao さんは書きました:
On 2020/11/16 12:22, Seino Yuki wrote:
Thanks for updating the patch!
+ pgss_info->dealloc = 0;
+ SpinLockInit(&pgss_info->mutex);
+ Assert(pgss_info->dealloc == 0);
Why is this assertion check necessary? It seems not necessary.
+ {
+ Assert(found == found_info);
Having pgssSharedState and pgssInfoCounters separately might make
the code a bit more complicated like the above? If this is true, what
about
including pgssInfoCounters in pgssSharedState?
PGSS_FILE_HEADER needs to be changed since the patch changes
the format of pgss file?
+ /* Read pgss_info */
+ if (feof(file) == 0)
+ if (fread(pgss_info, sizeof(pgssInfoCounters), 1, file) !=
1)
+ goto read_error;
Why does feof(file) need to be called here?
+pgss_info_update(void)
+{
+ {
Why is the second "{" necessary? It seems redundant.
+pgss_info_reset(void)
+{
+ {
Same as above.
+pg_stat_statements_info(PG_FUNCTION_ARGS)
+{
+ int64 d_count = 0;
+ {
Same as above.
+ SpinLockAcquire(&c->mutex);
+ d_count = Int64GetDatum(c->dealloc);
+ SpinLockRelease(&c->mutex);
Why does Int64GetDatum() need to be called here? It seems not
necessary.
+ <varlistentry>
+ <term>
+ <function>pg_stat_statements_info() returns bigint</function>
+ <indexterm>
+ <primary>pg_stat_statements_info</primary>
+ </indexterm>
+ </term>
Isn't it better not to expose pg_stat_statements_info() function in
the
document because pg_stat_statements_info view is enough and there
seems no use case for the function?
Regards,
Thanks for the comment.
I'll post a fixed patch.
Due to similar fixed, we have also merged the patches discussed in the
following thread.
https://commitfest.postgresql.org/30/2738/
I agree that these two patches should use the same infrastructure
because they both try to add the global stats for pg_stat_statements.
But IMO they should not be merged to one patch. It's better to
develop them one by one for ease of review. Thought?
So I extracted the "dealloc" part from the merged version of your
patch.
Also I refactored the code and applied some cosmetic changes into
the patch. Attached is the updated version of the patch that implements
only "dealloc" part. Could you review this version?
Regards,
Thank you for posting the new patch.
I checked "regression test" and "document" and "operation of the view".
No particular problems were found.
I just want to check one thing: will the log output be unnecessary this
time?
Quotes from v2.patch
+ {
entry_dealloc();
+ /* Update pgss_info */
+ {
+ volatile pgssSharedState *s = (volatile pgssSharedState
*) pgss;
+ SpinLockAcquire(&s->mutex);
+ s->pgss_info.dealloc += 1; /* increment dealloc count
*/
+ SpinLockRelease(&s->mutex);
+ }
+ ereport(LOG,
+ (errmsg("The information in pg_stat_statements has been
deallocated.")));
+ }
Regards.
