Re: [PATCH] Add features to pg_stat_statements - Mailing list pgsql-hackers
From | Fujii Masao |
---|---|
Subject | Re: [PATCH] Add features to pg_stat_statements |
Date | |
Msg-id | 2e9d596e-5b33-3368-682b-87431b65ce8f@oss.nttdata.com Whole thread Raw |
In response to | Re: [PATCH] Add features to pg_stat_statements (Seino Yuki <seinoyu@oss.nttdata.com>) |
Responses |
Re: [PATCH] Add features to pg_stat_statements
(Seino Yuki <seinoyu@oss.nttdata.com>)
|
List | pgsql-hackers |
On 2020/11/09 18:02, Seino Yuki wrote: > 2020-11-09 15:39 に Seino Yuki さんは書きました: >>>> >>>> However, let me confirm the following. >>>>> Is this information really useful? >>>>> If there is no valid use case for this, I'd like to drop it. >>>>> Thought? >>>> >>>> I thought it would be easy for users to see at a glance that if there is a case I assumed, >>>> if the last modified date and time is old, there is no need to adjust at all, and if the >>>> last modified date and time is recent, it would be easy for users to understand that the >>>> parameters need to be adjusted. >>>> What do you think? >>> >>> Even without the last deallocation timestamp, you can presume >>> when the deallocation of entries happened by periodically monitoring >>> the total number of deallocations and seeing those history. Or IMO >>> it's better to log whenever the deallocation happens as proposed upthread. >>> Then you can easily check the history of occurrences of deallocations >>> from the log file. >>> >>> Regards, >> >> +1.I think you should output the deallocation history to the log as well. >> >> In light of the above, I've posted a patch that reflects the points made. >> >> Regards, > > Sorry. There was a typo in the documentation correction. > I'll post a patch to fix it. 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, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
pgsql-hackers by date: