Re: Fix inconsistencies for v12

28.05.2019 2:05, Amit Kapila wrote:
> On Mon, May 27, 2019 at 3:59 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Amit Kapila <amit.kapila16@gmail.com> writes:
>>> On Sun, May 26, 2019 at 2:20 AM Alexander Lakhin <exclusion@gmail.com> wrote:
>>>> 5. ExecContextForcesOids - not changed, but may be should be removed
>>>> (orphaned after 578b2297)
>>> Yes, we should remove the use of ExecContextForcesOids.
>> Unless grep is failing me, ExecContextForcesOids is in fact gone.
>> All that's left is one obsolete mention in a comment, which should
>> certainly be cleaned up.
>>
> That's right and I was talking about that usage.  Initially, I thought
> we need to change the comment, but on again looking at the code,  I
> think we can remove that comment and the related code, but I am not
> completely sure.  If we read the comment atop ExecContextForcesOids
> [1] before it was removed, it seems to indicate that the
> initialization of es_result_relation_info for each subplan is for its
> usage in ExecContextForcesOids.  I have run the regression tests with
> the attached patch (which removes changing es_result_relation_info in
> ExecInitModifyTable) and all the tests passed.  Do you have any
> thoughts on this matter?
>
>
> [1]
> /*
>  ..
>  * We assume that if we are generating tuples for INSERT or UPDATE,
>  * estate->es_result_relation_info is already set up to describe the target
>  * relation.  Note that in an UPDATE that spans an inheritance tree, some of
>  * the target relations may have OIDs and some not.  We have to make the
>  * decisions on a per-relation basis as we initialize each of the subplans of
>  * the ModifyTable node, so ModifyTable has to set es_result_relation_info
>  * while initializing each subplan.
> ..
> */
I got a coredump with `make installcheck-world` (on postgres_fdw test):
Core was generated by `postgres: law contrib_regression [local]
UPDATE                               '.
Program terminated with signal SIGSEGV, Segmentation fault.
#0  0x00007ff1410ece98 in postgresBeginDirectModify
(node=0x560a563fab30, eflags=0) at postgres_fdw.c:2363
2363            rtindex =
estate->es_result_relation_info->ri_RangeTableIndex;
(gdb) bt
#0  0x00007ff1410ece98 in postgresBeginDirectModify
(node=0x560a563fab30, eflags=0) at postgres_fdw.c:2363
#1  0x0000560a55979e62 in ExecInitForeignScan
(node=node@entry=0x560a56254dc0, estate=estate@entry=0x560a563f9ae8,
    eflags=eflags@entry=0) at nodeForeignscan.c:227
#2  0x0000560a5594e123 in ExecInitNode (node=node@entry=0x560a56254dc0,
estate=estate@entry=0x560a563f9ae8,
    eflags=eflags@entry=0) at execProcnode.c:277
...
So It seems that this is not a dead code.

This comment initially appeared with c7a165ad in
nodeAppend.c:ExecInitAppend as following:
        /*
         * call ExecInitNode on each of the plans to be executed and
save the
         * results into the array "initialized".  Note we *must* set
         * estate->es_result_relation_info correctly while we initialize
each
         * sub-plan; ExecAssignResultTypeFromTL depends on that!
         */
        for (i = appendstate->as_firstplan; i <=
appendstate->as_lastplan; i++)
        {
                appendstate->as_whichplan = i;
                exec_append_initialize_next(node);

                initNode = (Plan *) nth(i, appendplans);
                initialized[i] = ExecInitNode(initNode, estate, (Plan *)
node);
        }

        /*
         * initialize tuple type
         */
        ExecAssignResultTypeFromTL((Plan *) node, &appendstate->cstate);
        appendstate->cstate.cs_ProjInfo = NULL;

and in ExecAssignResultTypeFromTL we see:
     * This is pretty grotty: we need to ensure that result tuples have
     * space for an OID iff they are going to be stored into a relation
     * that has OIDs.  We assume that estate->es_result_relation_info
     * is already set up to describe the target relation.

So the initial comment stated that before calling
ExecAssignResultTypeFromTL we need to have correct
es_result_relation_infos (but we don't set them in that code).

Later in commit a376a467 we have the ExecContextForcesOids call inside
ExecAssignResultTypeFromTL appeared:
void
ExecAssignResultTypeFromTL(PlanState *planstate)
{
        bool            hasoid;
        TupleDesc       tupDesc;

        if (ExecContextForcesOids(planstate, &hasoid))
        {
                /* context forces OID choice; hasoid is now set correctly */
        }
And the comment was changed to:
            Note we *must* set
         * estate->es_result_relation_info correctly while we initialize
each
         * sub-plan; ExecContextForcesOids depends on that!

although the code still calls ExecAssignResultTypeFromTL:
        for (i = appendstate->as_firstplan; i <=
appendstate->as_lastplan; i++)
        {
                appendstate->as_whichplan = i;
                exec_append_initialize_next(appendstate);

                initNode = (Plan *) nth(i, node->appendplans);
                appendplanstates[i] = ExecInitNode(initNode, estate);
        }

        /*
         * initialize tuple type
         */
        ExecAssignResultTypeFromTL(&appendstate->ps);

Later, in 8a5849b7 the comment moves out of nodeAppend.c:ExecInitAppend
into nodeModifyTable.c: ExecInitModifyTable (where we see it now):
        /*
         * call ExecInitNode on each of the plans to be executed and
save the
         * results into the array "mt_plans".  Note we *must* set
         * estate->es_result_relation_info correctly while we initialize
each
         * sub-plan; ExecContextForcesOids depends on that!
         */
        estate->es_result_relation_info = estate->es_result_relations;
        i = 0;
        foreach(l, node->plans)
        {
                subplan = (Plan *) lfirst(l);
                mtstate->mt_plans[i] = ExecInitNode(subplan, estate,
eflags);
                estate->es_result_relation_info++;
                i++;
        }
        estate->es_result_relation_info = NULL;

This code actually sets es_result_relation_info, but
ExecAssignResultTypeFromTL not called there anymore. So it seems that
this comment at least diverged from the initial author's intent.
With this in mind, I am inclined to just remove it.

(On a side note, I agree with your remarks regarding 2 and 3; please
look at a better patch for 3 attached.)

Best regards,
Alexander