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

Re: ProcessUtility_hook

From: Bruce Momjian <bruce(at)momjian(dot)us>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Itagaki Takahiro <itagaki(dot)takahiro(at)oss(dot)ntt(dot)co(dot)jp>, Euler Taveira de Oliveira <euler(at)timbira(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: ProcessUtility_hook
Date: 2009-12-01 02:32:19
Message-ID: (view raw, whole thread or download thread mbox)
Lists: pgsql-hackers
OK, reverted and placed back in "Needs Review" status.


Tom Lane wrote:
> I wrote:
> > It wasn't marked Ready For Committer, so presumably the reviewer
> > wasn't done with it.  I know I hadn't looked at it at all, because
> > I was waiting for the commitfest review process to finish.
> ... 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.
> 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"?
> 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.
> 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.
> 			regards, tom lane

  Bruce Momjian  <bruce(at)momjian(dot)us>

  + If your life is a hard drive, Christ can be your backup. +

In response to

pgsql-hackers by date

Next:From: Bruce MomjianDate: 2009-12-01 02:36:31
Subject: Re: ProcessUtility_hook
Previous:From: Tom LaneDate: 2009-12-01 02:24:59
Subject: Re: ProcessUtility_hook

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