Re: Feature improvement for pg_stat_statements - Mailing list pgsql-hackers

From Seino Yuki
Subject Re: Feature improvement for pg_stat_statements
Date
Msg-id 4baf35404ae24fe28b2623fbd5774bd0@oss.nttdata.com
Whole thread Raw
In response to Re: Feature improvement for pg_stat_statements  (Kyotaro Horiguchi <horikyota.ntt@gmail.com>)
Responses Re: Feature improvement for pg_stat_statements  (Seino Yuki <seinoyu@oss.nttdata.com>)
List 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

pgsql-hackers by date:

Previous
From: Anastasia Lubennikova
Date:
Subject: Re: Asymmetric partition-wise JOIN
Next
From: Magnus Hagander
Date:
Subject: Re: pg_upgrade analyze script