From: | Seino Yuki <seinoyu(at)oss(dot)nttdata(dot)com> |
---|---|
To: | Kyotaro Horiguchi <horikyota(dot)ntt(at)gmail(dot)com> |
Cc: | pgsql-hackers(at)lists(dot)postgresql(dot)org |
Subject: | Re: Feature improvement for pg_stat_statements |
Date: | 2020-11-09 11:09:29 |
Message-ID: | 4baf35404ae24fe28b2623fbd5774bd0@oss.nttdata.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Thank you for pointing that out. I'll post a fixed patch.
> + SpinLockAcquire(&pgss->mutex);
>
> You might noticed, but there a purpose of using the following
> idiom. Without that, compiler might optimize out the comparison
> assuming *pgss won't change.
>
>> volatile pgssSharedState *s = (volatile pgssSharedState *) pgss; \
>> SpinLockAcquire(&s->mutex); \
>
> + SpinLockAcquire(&pgss->mutex);
> + pgss->reset_time = GetCurrentTimestamp();
Fix the use of this idiom when modifying *pgss.
> We should avoid (possiblly) time-cosuming thing like
> GetCurrentTimestamp();
I understood your point to be "It's better not to execute
GetCurrentTimestamp()
while spinlock is running.
Fix to store the result of GetCurrentTimestamp() in a local variable
before
running spinlock. This value is stored in reset_time.
> + this function shows last reset time only when
> <function>pg_stat_statements_reset(0,0,0)</function> is called.
>
> This reads as "この関数はpg_statstatement_reset(0,0,0) が呼び出された
> ときにだけ最終リセット時刻を表示します。", which I think is different
> from what is intentended.
>
> And the wording is not alike with the documentation for similar
> functions.
>
> https://www.postgresql.org/docs/13/functions-datetime.html
>> current_timestamp → timestamp with time zone
>> Current date and time (start of current transaction); see Section
>> 9.9.4
>
> https://www.postgresql.org/docs/13/monitoring-stats.html
> pg_stat_archiver view
>> stats_reset timestamp with time zone
>> Time at which these statistics were last reset
>
> So something like the following?
>
> Time at which pg_stat_statements_reset(0,0,0) was last called.
>
> or
>
> Time at which statistics are last discarded by calling
> pg_stat_statements_reset(0,0,0).
I have made the following corrections.
this function shows last reset time only when
<function>pg_stat_statements_reset(0,0,0)</function> is called.
→this function shows the time at which
<function>pg_stat_statements_reset(0,0,0)</function> was last called.
<function>pg_stat_statements_reset_time(void) returns time stamp with
time zone</function>
→<function>pg_stat_statements_reset_time(void) returns timestamp with
time zone</function>
Regards.
Attachment | Content-Type | Size |
---|---|---|
pg_stat_statements_reset_time_v2.patch | text/x-diff | 6.3 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Magnus Hagander | 2020-11-09 11:18:44 | Re: pg_upgrade analyze script |
Previous Message | Anastasia Lubennikova | 2020-11-09 10:53:47 | Re: Asymmetric partition-wise JOIN |