Horton

Re: Command Triggers, patch v11

Andres Freund <andres@anarazel.de>

View thread
From: Andres Freund <andres@anarazel.de>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: pgsql-hackers@postgresql.org, Thom Brown <thom@linux.com>, Dimitri Fontaine <dimitri@2ndquadrant.fr>
Date: 2012-03-16T21:07:23Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Fix SPGiST vacuum algorithm to handle concurrent tuple motion properly.

  2. Remove useless const qualifier

  3. Make executor's SELECT INTO code save and restore original tuple receiver.

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