Re: contrib/pg_stat_statements 1212 - Mailing list pgsql-hackers

On Mon, Dec 22, 2008 at 00:44, ITAGAKI Takahiro
<itagaki.takahiro@oss.ntt.co.jp> wrote:
>
> "Alex Hunsaker" <badalex@gmail.com> wrote:
>
>> A few comments:
>>
>> Is there a reason you add sourceText to QueryDesc? AFAICT you can do
>> ActivePortal->sourceText and it will always be populated correctly.
>
> That's for nested statements (SQLs called in stored functions).
> ActivePortal->sourceText shows text of only top most query.
>
>> I think the explain_analyze_format guc is a clever way of getting
>> around the explain analyze verbose you proposed earlier.  But I dont
>> see any doc updates for it.
>
> Sure, no docs for now. The guc approach is acceptable?
> (I'm not sure whether it is the best way...)
> If ok, I'll write docs for it.

I dunno, Im hopping that splitting up the patches and making the
change more visible some more people might chime in :)

>> Im still not overly fond of the "statistics." custom guc name, but
>> what can you do...
>
> I have no obsessions with the name. The "pg_stat_statements.*" might
> be better to avoid confliction of prefix. If so, we'd better to rename
> variables to kill duplication of "statements" from the names.
>
> Ex.
>    statistics.max_statements   -> pg_stat_statements.limit
>    statistics.track_statements -> pg_stat_statements.target

How about just pg_stat_statements.track ?

>    statistics.saved_file       -> pg_stat_statements.saved_file

I do like the consistency of having the custom gucs be the same as the
module name, easy to grep or google for.

>> Other than that it looks good, though I admit I have not had the time
>> to sit down and thoroughly test it yet...
>
> I found another bug in my patch.
>
> [pg_stat_statements-1212.patch # pg_stat_statements()]
>    SpinLockAcquire(&entry->mutex);
>    values[i++] = Int64GetDatumFast(entry->counters.calls);
>    values[i++] = Float8GetDatumFast(entry->counters.total_time);
>    values[i++] = Float8GetDatumFast(entry->counters.cpu_user);
>    values[i++] = Float8GetDatumFast(entry->counters.cpu_sys);
>    values[i++] = Int64GetDatumFast(entry->counters.gets);
>    values[i++] = Int64GetDatumFast(entry->counters.reads);
>    values[i++] = Int64GetDatumFast(entry->counters.lreads);
>    values[i++] = Int64GetDatumFast(entry->counters.rows);
>    SpinLockRelease(&entry->mutex);
>
> The variables are not protected by spinlock actually when float64 and
> int64 are passed by reference (especially on 32bit platform).
> It'd be better to copy values:
>
>    Counters    tmp;
>    /* copy the actual values in spinlock */
>    SpinLockAcquire(&entry->mutex);
>    tmp = entry->counters;
>    SpinLockRelease(&entry->mutex);
>    /* create a tuple after lock is released. */
>    values[i++] = Int64GetDatumFast(tmp.calls);
>    values[i++] = Float8GetDatumFast(tmp.total_time);
>    ...

Ive only been testing on 64bit... maybe thats why I never ran into this.