To: Andres Freund <andres@anarazel.de>
cc: pgsql-hackers@postgresql.org, Thom Brown <thom@linux.com>,
	Dimitri Fontaine <dimitri@2ndquadrant.fr>
Subject: Re: Command Triggers, patch v11 
In-reply-to: <201203162237.38813.andres@anarazel.de> 
References: <m24nufq466.fsf@2ndQuadrant.fr>
	<201203162207.23896.andres@anarazel.de>
	<21494.1331933517@sss.pgh.pa.us>
	<201203162237.38813.andres@anarazel.de>
Comments: In-reply-to Andres Freund <andres@anarazel.de>
	message dated "Fri, 16 Mar 2012 22:37:38 +0100"
Date: Fri, 16 Mar 2012 17:52:55 -0400
Message-ID: <21818.1331934775@sss.pgh.pa.us>
From: Tom Lane <tgl@sss.pgh.pa.us>

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.

(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.  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.  (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?

			regards, tom lane