Query::targetList and RETURNING

Marko Tiikkaja <marko.tiikkaja@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

(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

Marko Tiikkaja <marko.tiikkaja@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

On Tue, Nov 10, 2009 at 2:56 PM, Marko Tiikkaja
<marko.tiikkaja@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

Tom Lane wrote:

Marko Tiikkaja <marko.tiikkaja@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

Marko Tiikkaja <marko.tiikkaja@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

Tom Lane wrote:

Marko Tiikkaja <marko.tiikkaja@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

Marko Tiikkaja <marko.tiikkaja@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