Re: [PATCH] Add features to pg_stat_statements - Mailing list pgsql-hackers
From | Seino Yuki |
---|---|
Subject | Re: [PATCH] Add features to pg_stat_statements |
Date | |
Msg-id | f34470e5e1d409a99eb1617197a16102@oss.nttdata.com Whole thread Raw |
In response to | Re: [PATCH] Add features to pg_stat_statements (Fujii Masao <masao.fujii@oss.nttdata.com>) |
Responses |
Re: [PATCH] Add features to pg_stat_statements
(Fujii Masao <masao.fujii@oss.nttdata.com>)
|
List | pgsql-hackers |
> 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/ > Why is this assertion check necessary? It seems not necessary. As indicated, it is unnecessary and will be removed. > 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? Fix pgssSharedState to include pgssInfoCounters . The related parts were also corrected accordingly. > PGSS_FILE_HEADER needs to be changed since the patch changes > the format of pgss file? The value of PGSS_FILE_HEADER has been updated. > Why does feof(file) need to be called here? As indicated, it is unnecessary and will be removed. > Why is the second "{" necessary? It seems redundant. As indicated, it is unnecessary and will be removed. But I left the {} in pg_stat_statements_info() to make the shared memory edit part explicit. > Why does Int64GetDatum() need to be called here? It seems not > necessary. As indicated, it is unnecessary and will be removed. > 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? As indicated, it is unnecessary and will be removed. Regards.
Attachment
pgsql-hackers by date: