Re: [HACKERS] Query::targetList and RETURNING
Marko Tiikkaja marko.tiikk...@cs.helsinki.fi writes: While working on writeable CTEs, I noticed I have to special-case the output of a Query node frequently because in INSERT/UPDATE/DELETE query targetList is the target list which is used for modifying the result relation and returningList is the output of that Query. However, this is different from SELECT where targetList actually is the output of that Query node. Attached is a patch which avoids this special-casing by making Query's targetList always be the output target list. The target list for the result relation is stored separately. The patch needs a bit more work but I'll be glad to do it if people think this is useful. This doesn't really seem like a good idea from here. You're changing a decision that has something like twenty years' standing in the code, for no real gain. AFAICS this is just going to move the special cases from point A to point B. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Query::targetList and RETURNING
(Sorry, forgot to CC the list) Tom Lane wrote: This doesn't really seem like a good idea from here. You're changing a decision that has something like twenty years' standing in the code, for no real gain. AFAICS this is just going to move the special cases from point A to point B. Right, but this way you only have to special-case in grouping_planner(), and targetList always means the same thing. Regards, Marko Tiikkaja -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Query::targetList and RETURNING
Marko Tiikkaja marko.tiikk...@cs.helsinki.fi writes: Tom Lane wrote: This doesn't really seem like a good idea from here. You're changing a decision that has something like twenty years' standing in the code, for no real gain. AFAICS this is just going to move the special cases from point A to point B. Right, but this way you only have to special-case in grouping_planner(), and targetList always means the same thing. If you think that, it just means you have not found all the places you need to special-case ;-). One really obvious example is ruleutils.c, and I rather imagine there are multiple places in the parser and rewriter that would need attention, quite aside from whatever it does to the planner. If there were a clear net benefit, I'd be for changing, but I think it's going to end up being roughly a wash. And if it's a wash we should not change it, because when you consider the follow-on costs (patches not back-patching, third-party code breaking, etc) that means we'd come out way behind. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Query::targetList and RETURNING
On Tue, Nov 10, 2009 at 2:56 PM, Marko Tiikkaja marko.tiikk...@cs.helsinki.fi wrote: (Sorry, forgot to CC the list) Tom Lane wrote: This doesn't really seem like a good idea from here. You're changing a decision that has something like twenty years' standing in the code, for no real gain. AFAICS this is just going to move the special cases from point A to point B. Right, but this way you only have to special-case in grouping_planner(), and targetList always means the same thing. I haven't read through the patch but I can say the existing arrangement certainly seemed strange to me when I first saw it. -- greg -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Query::targetList and RETURNING
Tom Lane wrote: Marko Tiikkaja marko.tiikk...@cs.helsinki.fi writes: Tom Lane wrote: This doesn't really seem like a good idea from here. You're changing a decision that has something like twenty years' standing in the code, for no real gain. AFAICS this is just going to move the special cases from point A to point B. Right, but this way you only have to special-case in grouping_planner(), and targetList always means the same thing. If you think that, it just means you have not found all the places you need to special-case ;-). One really obvious example is ruleutils.c, and I rather imagine there are multiple places in the parser and rewriter that would need attention, quite aside from whatever it does to the planner. Of course there were multiple places that needed attention, but those don't look like special-casing to me, they just have to make sure to do what they need to on the correct list of those two, which is what they did before. I wouldn't care for this at all, but with things the way they are right now, the writeable CTE patch has to do quite a few of these: List *cteList; if (query-cmdType != CMD_SELECT) cteList = query-returningList; else cteList = query-targetList; /* do something with cteList */ With this patch, they could use targetList directly without paying any attention to the type of the Query inside them. Regards, Marko Tiikkaja -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Query::targetList and RETURNING
Marko Tiikkaja marko.tiikk...@cs.helsinki.fi writes: I wouldn't care for this at all, but with things the way they are right now, the writeable CTE patch has to do quite a few of these: [ shrug... ] How many is quite a few? In a quick search for existing references to targetList in the planner, it looked to me like the majority were places that wouldn't be relevant for writable CTEs anyway. For example, none of the references in allpaths.c are, because they have to do with deciding whether quals can be pushed down into the subquery. And the answer to that, for a non-SELECT CTE, is always no. if (query-cmdType != CMD_SELECT) cteList = query-returningList; else cteList = query-targetList; Just a thought ... where you do need this, would it be better to phrase it as if (query-returningList) cteList = query-returningList; else cteList = query-targetList; ? I'm not sure myself, but it's something to consider. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Query::targetList and RETURNING
Tom Lane wrote: Marko Tiikkaja marko.tiikk...@cs.helsinki.fi writes: I wouldn't care for this at all, but with things the way they are right now, the writeable CTE patch has to do quite a few of these: [ shrug... ] How many is quite a few? In a quick search for existing references to targetList in the planner, it looked to me like the majority were places that wouldn't be relevant for writable CTEs anyway. For example, none of the references in allpaths.c are, because they have to do with deciding whether quals can be pushed down into the subquery. And the answer to that, for a non-SELECT CTE, is always no. It appears we have four of those at the moment (hmm.. I thought there were more). Just a thought ... where you do need this, would it be better to phrase it as if (query-returningList) cteList = query-returningList; else cteList = query-targetList; ? I'm not sure myself, but it's something to consider. My initial thought is that this won't work because there might not be a RETURNING clause, but I'm not sure. Regards, Marko Tiikkaja -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Query::targetList and RETURNING
Marko Tiikkaja marko.tiikk...@cs.helsinki.fi writes: Tom Lane wrote: if (query-returningList) cteList = query-returningList; else cteList = query-targetList; My initial thought is that this won't work because there might not be a RETURNING clause, but I'm not sure. Hm, would we allow DML without RETURNING in CTEs? I'm not sure I see the point of that. But in any case, the other way is fine. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
