From: | Itagaki Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | Bruce Momjian <bruce(at)momjian(dot)us>, Euler Taveira de Oliveira <euler(at)timbira(dot)com>, pgsql-hackers(at)postgresql(dot)org |
Subject: | Re: ProcessUtility_hook |
Date: | 2009-12-03 07:05:47 |
Message-ID: | 20091203160546.88D3.52131E4D@oss.ntt.co.jp |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> ... and now that I have, I find at least four highly questionable
> things about it:
>
> 1. The placement of the hook. Why is it three lines down in
> ProcessUtility? It's probably reasonable to have the Assert first,
> but I don't see why the hook function should have the ability to
> editorialize on the behavior of everything about ProcessUtility
> *except* the read-only-xact check.
I moved the initialization of completionTag into standard_ProcessUtility.
> 2. The naming and documentation of the added GUC setting for
> pg_stat_statements. "track_ddl" seems pretty bizarre to me because
> there are many utility statements that no one would call DDL. COPY,
> for example, is certainly not DDL. Why not call it "track_utility"?
Ok, fixed.
> 3. The enable-condition test in pgss_ProcessUtility. Is it really
> appropriate to be gating this by isTopLevel? I should think that
> the nested_level check in pgss_enabled would be sufficient and
> more likely to do what's expected.
I removed the isTopLevel check. I was worried about auto-generated
utility commands; generated sub commands are called with the same
query string as the top query. Don't it confuse statistics?
> 4. The special case for CopyStmt. That's just weird, and it adds
> a maintenance requirement we don't need. I don't see a really good
> argument why COPY (alone among utility statements) deserves to have
> a rowcount tracked by pg_stat_statements, but even if you want that
> it'd be better to rely on examining the completionTag after the fact.
> The fact that the tag is "COPY nnnn" is part of the user-visible API
> for COPY and won't change lightly. The division of labor between
> ProcessUtility and copy.c is far more volatile, but this patch has
> injected itself into that.
Ok, fixed. I've thought string-based interface is not desirable, but it
should be a stable API. COPY and INSERT/UPDATE/DELETE (used by EXECUTE)
are counted by pg_stat_statements, but EXECUTE SELECT is impossible.
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
Attachment | Content-Type | Size |
---|---|---|
ProcessUtility_hook_20091203.patch | application/octet-stream | 11.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Kern Sibbald | 2009-12-03 07:33:38 | Re: Catastrophic changes to PostgreSQL 8.4 |
Previous Message | Hitoshi Harada | 2009-12-03 06:58:39 | Re: Cost of sort/order by not estimated by the query planner |