Skip site navigation (1) Skip section navigation (2)

Re: contrib/pg_stat_statements 1212

From: ITAGAKI Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>
To: "Alex Hunsaker" <badalex(at)gmail(dot)com>
Cc: pgsql-hackers(at)postgresql(dot)org
Subject: Re: contrib/pg_stat_statements 1212
Date: 2008-12-22 07:44:01
Message-ID: 20081222162154.8485.52131E4D@oss.ntt.co.jp (view raw or flat)
Thread:
Lists: pgsql-hackers
"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



In response to

Responses

pgsql-hackers by date

Next:From: Grzegorz JaskiewiczDate: 2008-12-22 07:51:19
Subject: affected rows count
Previous:From: Zdenek KotalaDate: 2008-12-22 07:43:32
Subject: Re: reloptions and toast tables

Privacy Policy | About PostgreSQL
Copyright © 1996-2014 The PostgreSQL Global Development Group