From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: Command Triggers, patch v11
Date: Fri, 16 Mar 2012 23:17:43 +0100
User-Agent: KMail/1.13.7 (Linux/3.3.0-rc3-andres-00278-g4903062; KDE/4.6.5;
	x86_64; ; )
Cc: pgsql-hackers@postgresql.org, Thom Brown <thom@linux.com>,
	Dimitri Fontaine <dimitri@2ndquadrant.fr>
References: <m24nufq466.fsf@2ndQuadrant.fr>
	<201203162237.38813.andres@anarazel.de>
	<21818.1331934775@sss.pgh.pa.us>
In-Reply-To: <21818.1331934775@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: Text/Plain;
  charset="iso-8859-15"
Content-Transfer-Encoding: 7bit
Message-Id: <201203162317.43818.andres@anarazel.de>

On Friday, March 16, 2012 10:52:55 PM Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > On Friday, March 16, 2012 10:31:57 PM Tom Lane wrote:
> >> I'm thinking that if the table creation
> >> were to be moved into the tuple receiver's startup routine, we could
> >> avoid needing to get control back between ExecutorStartup and
> >> ExecutorRun, and then all that would be required would be to call
> >> ExecuteQuery with a different DestReceiver.
> > 
> > Hm. I seriously dislike doing that in the receiver. I can't really point
> > out why though. Unsurprisingly I like getting the control back to
> > CreateTableAs...
> 
> I don't see the argument.  Receiver startup functions are intended to do
> exactly this type of thing.  I'd be okay with leaving it in
> CreateTableAs if it didn't contort the code to do so, but what we have
> here is precisely that we've got to contort the interface with prepare.c
> to do it that way.
Hm.

> (It also occurs to me that moving this work into the DestReceiver might
> make things self-contained enough that we could continue to support
> EXPLAIN SELECT INTO with not an undue amount of pain.)

> Something else I just came across is that there are assorted places that
> are aware that ExplainStmt contains a Query, eg setrefs.c, plancache.c,
> and those have got to treat CreateTableAsStmt similarly.
Hm. Is that so? As implemented in my version the planner just sees a plain 
statement instead of a utility statement. Am I missing something?

I am not even sure why the planner ever needs to see ExplainStmts? Protocol 
level prepares? Shouldn't those top level nodes be simply removed once?

> We could just
> add more code in each of those places.  I'm wondering though if it would
> be a good idea to invent an abstraction layer, to wit a utility.c
> function along the lines of "Query *UtilityContainsQuery(Node
> *parsetree)", which would centralize the knowledge of exactly which
> utility statements are like this and how to dig the Query out of them.
> It's only marginally attractive unless there's a foreseeable reason
> why we'd someday have more than two of them; but on the other hand,
> just formalizing the concept that some utility statements are like
> this might be a good thing. 
If its really necessary to do that I think that would be a good idea. Alone 
the increased greppablility/readablility seems to be worth it.

> (Actually, as I type this I wonder whether
> COPY (SELECT ...) isn't a member of this class too, and whether we don't
> have bugs from the fact that it's not being treated like EXPLAIN.)
> Thoughts?
Hm. On a first glance the planner also never sees the content of a CopyStmt...

Andres