28.05.2019 2:05, Amit Kapila wrote:
> On Mon, May 27, 2019 at 3:59 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
>> Amit Kapila <amit.kapil...@gmail.com> writes:
>>> On Sun, May 26, 2019 at 2:20 AM Alexander Lakhin <exclus...@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
diff --git a/src/backend/access/heap/rewriteheap.c b/src/backend/access/heap/rewriteheap.c
index 131ec7b8d7..369694fa2e 100644
--- a/src/backend/access/heap/rewriteheap.c
+++ b/src/backend/access/heap/rewriteheap.c
@@ -350,7 +350,7 @@ end_heap_rewrite(RewriteState state)
*
* It's obvious that we must do this when not WAL-logging. It's less
* obvious that we have to do it even if we did WAL-log the pages. The
- * reason is the same as in tablecmds.c's copy_relation_data(): we're
+ * reason is the same as in storage.c's RelationCopyStorage(): we're
* writing data that's not in shared buffers, and so a CHECKPOINT
* occurring during the rewriteheap operation won't have fsync'd data we
* wrote before the checkpoint.
