Re: contrib/pg_stat_statements 1212

On Mon, Dec 22, 2008 at 00:44, ITAGAKI Takahiro
<itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> wrote:
>
> "Alex Hunsaker" <badalex(at)gmail(dot)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.