Re: contrib/pg_stat_statements 1212

"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.

> 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
statistics.saved_file -> pg_stat_statements.saved_file

> 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);
...

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center