From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@postgresql.org, Thom Brown <thom@linux.com>
Cc: Dimitri Fontaine <dimitri@2ndquadrant.fr>
Subject: Re: Command Triggers, patch v11
Date: Fri, 16 Mar 2012 22:07:23 +0100
Message-ID: <201203162207.23896.andres@anarazel.de> (raw)
In-Reply-To: <20905.1331931287@sss.pgh.pa.us>
References: <m24nufq466.fsf@2ndQuadrant.fr>
<201203041650.39826.andres@anarazel.de>
<20905.1331931287@sss.pgh.pa.us>
On Friday, March 16, 2012 09:54:47 PM Tom Lane wrote:
> Andres Freund <andres@anarazel.de> writes:
> > [ ctas-01.patch ]
>
> I'm starting to look at this now.
Great!
> For a patch that's supposed to de-complicate things, it seems pretty messy
:-(
Yea. It started out simple but never stopped getting more complicated. Getting
rid of the duplication still seems to make sense to me though.
> One thing I soon found is that it lacks support for EXPLAIN SELECT INTO.
> That used to work, but now you get
>
> regression=# explain select * into foo from tenk1;
> ERROR: INTO is only allowed on first SELECT of UNION/INTERSECT/EXCEPT
> LINE 1: explain select * into foo from tenk1;
> ^
>
> and while fixing the parse analysis for that is probably not too hard,
> it would still fail to work nicely, since explain.c lacks support for
> CreateTableAsStmt utility statements.I think we'd have to invent
> something parallel to ExplainExecuteQuery to support this, and I really
> doubt that it's worth the trouble. Does anyone have a problem with
> desupporting the case?
I am all for removing it.
> Also, it seems to me that the patch is spending way too much effort on
> trying to exactly duplicate the old error messages for various flavors
> of "INTO not allowed here", and still not succeeding terribly well.
> I'm inclined to just have a one-size-fits-all message in
> transformSelectStmt, which will fire if intoClause hasn't been cleared
> before we get there; any objections?
I was/am decidedly unhappy about the whole error message stuff. Thats what
turned the patch from removing lines to making it bigger than before.
I tried to be compatible to make sure I understood what was happening. I am
fine with making it one simple error message.
> A couple of other cosmetic thoughts: I'm tempted to split the execution
> support out into a new file, rather than bloat tablecmds.c any further;
> and I'm wondering if the interface to EXECUTE INTO can't be simplified.
> (It may have been a mistake to remove the IntoClause in ExecuteStmt.)
Hm. I vote against keeping the IntoClause stuff in ExecuteStmt. Splitting
stuff from tablecmd.c seems sensible though.
One more thing I disliked quite a bit was the duplication of the EXECUTE
handling. Do you see a way to deduplicate that?
If you give me some hints about what to change I am happy to revise the patch
myself should you prefer that.
Andres
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: andres@anarazel.de, tgl@sss.pgh.pa.us, thom@linux.com, dimitri@2ndquadrant.fr
Subject: Re: Command Triggers, patch v11
In-Reply-To: <201203162207.23896.andres@anarazel.de>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox