Re: Writeable CTE patch

Marko Tiikkaja <marko(dot)tiikkaja(at)cs(dot)helsinki(dot)fi> writes:
> Attached is the latest version of the patch.

I started to look at this patch and soon noted that a very substantial
fraction of the delta had to do with getting rid of dependencies on
estate->es_result_relation_info. It seemed to me that if we were going
to do that, we should try to get rid of the field altogether, so I
looked at what that would take. Unfortunately, there's a problem with
that, which I think proves the patch's approach invalid as well. With
the patch's changes, the only remaining user of es_result_relation_info
is ExecContextForcesOids(), which is only called during plan node
initialization. The patch supposes that it's sufficient to set up
es_result_relation_info while a ModifyTable node is initializing its
child nodes. What this misses is EvalPlanQual, which can require
initialization of a new plan tree during execution. To make it work
we'd need to be sure to set up es_result_relation_info during execution
as well, which pretty much destroys any gain from the proposed
refactoring.

When I realized this, my first thought was that we might as well drop
all the proposed changes that involve avoiding use of
es_result_relation_info. I was wondering though whether you had a
functional reason for getting rid of them, or if it was just trying to
tidy the code a bit?

I did think of a plan B: we could get rid of ExecContextForcesOids()
altogether and let plan nodes always do whatever seems locally most
efficient about OIDs. The consequence of this would be that if we
are doing INSERT or SELECT INTO and the plan tree produces the wrong
has-OIDs state, we would need to insert a junkfilter to fix it, which
would represent processing that would be unnecessary if there was no
other reason to have a junkfilter (ie, no junk columns in the result).
This actually does not affect UPDATE, which always has a junk TID
column so always needs a junkfilter anyway; nor DELETE, which doesn't
need to produce tuples for insertion. At the time we put in
ExecContextForcesOids() it seemed that there was enough of a use-case
to justify klugery to avoid the extra filtering overhead. However,
since OIDs in user tables have been deprecated for several versions
now, I'm thinking that maybe the case doesn't arise often enough to
justify keeping such a wart in the executor.

Comments?

regards, tom lane