On Mon, Dec 22, 2008 at 00:44, ITAGAKI Takahiro <itagaki.takah...@oss.ntt.co.jp> wrote: > > "Alex Hunsaker" <bada...@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. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
