Re: UPDATE using sub selects

NikhilS <nikkhils(at)gmail(dot)com> writes:
> As per discussion on -hackers, a patch which allows updates to use
> subselects is attached with this mail.

I looked over this patch briefly, and I'm afraid it still needs a lot of
work.

The way you altered transformUpdateStmt is a mess; it's duplicating
logic and also using inapplicable tests. (In particular, invoking
checkInsertTargets on a subset of the complete target list is just not
sensible --- it's supposed to be catching conflicts. I think you really
don't need it at all anyway, because the needed tests are done
downstream in the rewriter.) I think the right way is to expand out the
row-assignment portions of the SET target list and then process the
whole SET list the same as before.

The real problem though is that the patch creates a new storage method
and processing path for ROWEXPR_SUBLINK sublinks that's got nothing to
do with the other kinds of sublinks. This is going to be a mess and a
fertile source of bugs. (You already have quite a few stemming from
having overlooked all the places that have to be touched to add a field
to Query, eg backend/nodes/, optimizer/util/clauses.c, ruleutils.c...)

The basic idea is not bad, but I think if we're going to make a list of
subqueries to attach to Query, we should try to do all the forms of
SubLink that way, not just one. From a logical point of view all the
single-result-row forms of SubLink are about the same, and we should
strive to unify them not make them even more different. Also there are
a lot of kluges that could be got rid of if subqueries were pulled out
of expression trees leaving only Param references behind. We'd need
something a bit more general than Param, perhaps, depending on how we
wanted to distinguish output values from different subqueries. Right
now we don't have to worry about that at parse time, but can leave it to
the planner to assign globally unique Param IDs; but that work would
have to be pushed further upstream. (BTW, I'm fairly certain your patch
won't work for multiple subselects in one UPDATE, because it fails to
deal with this point. The test cases in the patch do NOT exercise the
case because you don't have multiple multiple-output subselects in any
of them.)

I thought about trying to fix this stuff but soon concluded that it
was more work than I could justify spending on one patch during
commit fest. It has to be bounced back for now.

regards, tom lane