Re: [PATCH] Add features to pg_stat_statements
От | Fujii Masao |
---|---|
Тема | Re: [PATCH] Add features to pg_stat_statements |
Дата | |
Msg-id | 334b9b54-dcdb-5fc3-654c-b319da584e0b@oss.nttdata.com обсуждение исходный текст |
Ответ на | Re: [PATCH] Add features to pg_stat_statements (Fujii Masao <masao.fujii@oss.nttdata.com>) |
Список | pgsql-hackers |
On 2020/11/25 17:07, Fujii Masao wrote: > > > On 2020/11/25 15:40, Seino Yuki wrote: >> 2020-11-25 13:13 に Fujii Masao さんは書きました: >>> On 2020/11/25 12:02, Seino Yuki wrote: >>>> 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. >>> >>> Thanks for the review and test! >>> So you think that this patch can be marked as ready for committer? >>> >>> >>>> >>>> I just want to check one thing: will the log output be unnecessary this time? >>>> Quotes from v2.patch >>> >>> I'm not sure if it's really good idea to add this log message. >>> If we adopt that logging, in the system where pgss entries are deallocated >>> very frequently, that message also would be logged very frequently. >>> Such too many log messages might be noisy to users. To address this issue, >>> we may want to add new parameter that controls whether log message is >>> emitted or not when entries are deallocated. But that parameter sounds >>> too specific... Thought? >>> >>> Regards, >> >> >>> Thanks for the review and test! >>> So you think that this patch can be marked as ready for committer? >> >> Updated status to ready for committer. > > Thanks! I modified the docs a bit and pushed the patch. Thanks! Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
В списке pgsql-hackers по дате отправления: