Обсуждение: Fix inconsistencies for v12

Hello hackers,

Please also consider fixing the following inconsistencies found in new
v12 code:

1. AT_AddOids - remove (orphaned after 578b2297)
2. BeingModified ->TM_BeingModified (for consistency)
3. copy_relation_data -> remove (orphaned after d25f5191)
4. endblock -> endblk (an internal inconsistency)
5. ExecContextForcesOids - not changed, but may be should be removed
(orphaned after 578b2297)
6. ExecGetResultSlot - remove (not used since introduction in 1a0586de)
7. existedConstraints & partConstraint -> provenConstraint &
testConstraint (sync with implementation)
8. heap_parallelscan_initialize -> remove the sentence (changed in c2fe139c)
9. heap_rescan_set_params - remove (orphaned after c2fe139c)
10. HeapTupleSatisfiesSnapshot -> HeapTupleSatisfiesVisibility (an
internal inconsistency)
11. interpretOidsOption - remove (orphaned after 578b2297)
12. item_itemno -> iter_itemno (an internal inconsistency)
13. iterset_is_member -> intset_is_member (an internal inconsistency)
14. latestRemovedxids -> latestRemovedXids (an inconsistent case)
15. newrode -> newrnode (an internal inconsistency)
16. NextSampletuple -> NextSampleTuple (an inconsistent case)
17. oid_typioparam - remove? (orphaned after 578b2297)
18. recoveryTargetIsLatest - remove (orphaned after 2dedf4d9)
19. register_unlink -> register_unlink_segment (an internal inconsistency)
20. RelationGetOidIndex ? just to remove the paragraph (orphaned after
578b2297)
21. slot_getsomeattr -> checked in slot_getsomeattrs ? (an internal
inconsistency and questionable grammar)
22. spekToken -> specToken (an internal inconsistency)
23. SSLdone -> secure_done (sync with implementation)
24. stats_relation & keep_buf - remove (orphaned after 9a8ee1dc & 5db6df0c0)
25. SyncRequstHandler -> SyncRequestHandler (a typo)
26. table_needs_toast_table -> table_relation_needs_toast_table (an
internal inconsistency)
27. XactTopTransactionId -> XactTopFullTransactionId (an internal
inconsistency)

The separate patches for all the defects (except 5) are attached. In
case a single patch is preferable, I can produce it too.

Best regards,
Alexander

On Sun, May 26, 2019 at 2:20 AM Alexander Lakhin <exclusion@gmail.com> wrote:
>
> Hello hackers,
>
> Please also consider fixing the following inconsistencies found in new
> v12 code:
>
> 1. AT_AddOids - remove (orphaned after 578b2297)
> 2. BeingModified ->TM_BeingModified (for consistency)
>

/*
- * A tuple is locked if HTSU returns BeingModified.
+ * A tuple is locked if HTSU returns TM_BeingModified.
  */
  if (htsu == TM_BeingModified)

I think the existing comment is quite clear.  I mean we can change it
if we want, but I don't see the dire need to do it.


> 3. copy_relation_data -> remove (orphaned after d25f5191)

- * reason is the same as in tablecmds.c's copy_relation_data(): 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.
+ * reason is that 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.

It seems to me that the same thing is moved to storage.c's
RelationCopyStorage() in the commit mentioned by you.  So, isn't it
better to change the comment accordingly rather than entirely removing
the reference to a similar comment in another place?

> 4. endblock -> endblk (an internal inconsistency)
> 5. ExecContextForcesOids - not changed, but may be should be removed
> (orphaned after 578b2297)

Yes, we should remove the use of ExecContextForcesOids.  We are using
es_result_relation_info at other places as well, so I think we can
change the comment to indicate the same.

>
> The separate patches for all the defects (except 5) are attached. In
> case a single patch is preferable, I can produce it too.
>

It is okay, we can review the individual patches and then combine them
later.   I couldn't get a chance to review all of them yet.  Thanks
for working on this.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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.

However, the full context of the mention is

    /*
     * call ExecInitNode on each of the plans to be executed and save the
     * results into the array "mt_plans".  This is also a convenient place to
     * verify that the proposed target relations are valid and open their
     * indexes for insertion of new index entries.  Note we *must* set
     * estate->es_result_relation_info correctly while we initialize each
     * sub-plan; ExecContextForcesOids depends on that!
     */

which makes one wonder if the code to twiddle
estate->es_result_relation_info during subplan init is dead code.  If so
we probably ought to remove it, as it's surely confusing.  If it's not
dead, then this comment ought to be updated to explain the surviving
reason(s), not simply deleted.

            regards, tom lane

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.
..
*/

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

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

On 2019/05/28 14:00, Alexander Lakhin wrote:
> 28.05.2019 2:05, Amit Kapila wrote:
>> ... 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?
>
> 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.

> ... 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.

Seeing that the crash occurs due to postgres_fdw relying on
es_result_relation_info being set when initializing a "direct
modification" plan on foreign tables managed by it, we could change the
comment to say that instead.  Note that allowing "direct modification" of
foreign tables is a core feature, so there's no postgres_fdw-specific
behavior here; there may be other FDWs that support "direct modification"
plans and so likewise rely on es_result_relation_info being set.

How about:

diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index a3c0e91543..95545c9472 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2316,7 +2316,7 @@ ExecInitModifyTable(ModifyTable *node, EState
*estate, int eflags)
      * verify that the proposed target relations are valid and open their
      * indexes for insertion of new index entries.  Note we *must* set
      * estate->es_result_relation_info correctly while we initialize each
-     * sub-plan; ExecContextForcesOids depends on that!
+     * sub-plan; FDWs may depend on that.
      */
     saved_resultRelInfo = estate->es_result_relation_info;

Thanks,
Amit

On Tue, May 28, 2019 at 12:29 PM Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
> On 2019/05/28 14:00, Alexander Lakhin wrote:
> > 28.05.2019 2:05, Amit Kapila wrote:
> >> ... 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?
> >
> > 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.
>
> > ... 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.
>
> Seeing that the crash occurs due to postgres_fdw relying on
> es_result_relation_info being set when initializing a "direct
> modification" plan on foreign tables managed by it, we could change the
> comment to say that instead.  Note that allowing "direct modification" of
> foreign tables is a core feature, so there's no postgres_fdw-specific
> behavior here; there may be other FDWs that support "direct modification"
> plans and so likewise rely on es_result_relation_info being set.
>


Can we ensure some way that only FDW's rely on it not any other part
of the code?

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 2019/05/28 20:26, Amit Kapila wrote:
> On Tue, May 28, 2019 at 12:29 PM Amit Langote wrote:
>> Seeing that the crash occurs due to postgres_fdw relying on
>> es_result_relation_info being set when initializing a "direct
>> modification" plan on foreign tables managed by it, we could change the
>> comment to say that instead.  Note that allowing "direct modification" of
>> foreign tables is a core feature, so there's no postgres_fdw-specific
>> behavior here; there may be other FDWs that support "direct modification"
>> plans and so likewise rely on es_result_relation_info being set.
> 
> 
> Can we ensure some way that only FDW's rely on it not any other part
> of the code?

Hmm, I can't think of any way of doing than other than manual inspection.
We are sure that no piece of core code relies on it in the ExecInitNode()
code path.  Apparently FDWs may, as we've found out here.  Now that I've
looked around, maybe other loadable modules may too, by way of (only?)
Custom nodes.  I don't see any other way to hook into ExecInitNode(), so
maybe that's it.

So, maybe reword a bit as:

diff --git a/src/backend/executor/nodeModifyTable.c
b/src/backend/executor/nodeModifyTable.c
index a3c0e91543..95545c9472 100644
--- a/src/backend/executor/nodeModifyTable.c
+++ b/src/backend/executor/nodeModifyTable.c
@@ -2316,7 +2316,7 @@ ExecInitModifyTable(ModifyTable *node, EState
*estate, int eflags)
      * verify that the proposed target relations are valid and open their
      * indexes for insertion of new index entries.  Note we *must* set
      * estate->es_result_relation_info correctly while we initialize each
-     * sub-plan; ExecContextForcesOids depends on that!
+     * sub-plan; external modules such as FDWs may depend on that.

Thanks,
Amit

On Tue, May 28, 2019 at 10:30 AM Alexander Lakhin <exclusion@gmail.com> wrote:
>
> 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.
> >>
..
> > */
> I got a coredump with `make installcheck-world` (on postgres_fdw test):
>

Thanks for noticing this.  I have run the tests in parallel mode with
something like make -s check-world -j4 PROVE_FLAGS='-j4'.  It didn't
stop at failure, so I missed to notice it.  However, now looking
carefully (by redirecting the output to a log file), I could see this.

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

The new patch looks good to me.  However, instead of committing just
this one alone, I will review others as well and see which all can be
combined and pushed together.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Wed, May 29, 2019 at 6:12 AM Amit Langote
<Langote_Amit_f8@lab.ntt.co.jp> wrote:
>
> On 2019/05/28 20:26, Amit Kapila wrote:
> > On Tue, May 28, 2019 at 12:29 PM Amit Langote wrote:
> >> Seeing that the crash occurs due to postgres_fdw relying on
> >> es_result_relation_info being set when initializing a "direct
> >> modification" plan on foreign tables managed by it, we could change the
> >> comment to say that instead.  Note that allowing "direct modification" of
> >> foreign tables is a core feature, so there's no postgres_fdw-specific
> >> behavior here; there may be other FDWs that support "direct modification"
> >> plans and so likewise rely on es_result_relation_info being set.
> >
> >
> > Can we ensure some way that only FDW's rely on it not any other part
> > of the code?
>
> Hmm, I can't think of any way of doing than other than manual inspection.
> We are sure that no piece of core code relies on it in the ExecInitNode()
> code path.  Apparently FDWs may, as we've found out here.  Now that I've
> looked around, maybe other loadable modules may too, by way of (only?)
> Custom nodes.  I don't see any other way to hook into ExecInitNode(), so
> maybe that's it.
>
> So, maybe reword a bit as:
>
> diff --git a/src/backend/executor/nodeModifyTable.c
> b/src/backend/executor/nodeModifyTable.c
> index a3c0e91543..95545c9472 100644
> --- a/src/backend/executor/nodeModifyTable.c
> +++ b/src/backend/executor/nodeModifyTable.c
> @@ -2316,7 +2316,7 @@ ExecInitModifyTable(ModifyTable *node, EState
> *estate, int eflags)
>       * verify that the proposed target relations are valid and open their
>       * indexes for insertion of new index entries.  Note we *must* set
>       * estate->es_result_relation_info correctly while we initialize each
> -     * sub-plan; ExecContextForcesOids depends on that!
> +     * sub-plan; external modules such as FDWs may depend on that.
>

I think it will be better to include postgres_fdw in the comment in
some way so that if someone wants a concrete example, there is
something to refer to.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On 2019/05/30 18:51, Amit Kapila wrote:
> On Wed, May 29, 2019 at 6:12 AM Amit Langote wrote:
>> On 2019/05/28 20:26, Amit Kapila wrote:
>>> Can we ensure some way that only FDW's rely on it not any other part
>>> of the code?
>>
>> Hmm, I can't think of any way of doing than other than manual inspection.
>> We are sure that no piece of core code relies on it in the ExecInitNode()
>> code path.  Apparently FDWs may, as we've found out here.  Now that I've
>> looked around, maybe other loadable modules may too, by way of (only?)
>> Custom nodes.  I don't see any other way to hook into ExecInitNode(), so
>> maybe that's it.
>>
>> So, maybe reword a bit as:
>>
>> diff --git a/src/backend/executor/nodeModifyTable.c
>> b/src/backend/executor/nodeModifyTable.c
>> index a3c0e91543..95545c9472 100644
>> --- a/src/backend/executor/nodeModifyTable.c
>> +++ b/src/backend/executor/nodeModifyTable.c
>> @@ -2316,7 +2316,7 @@ ExecInitModifyTable(ModifyTable *node, EState
>> *estate, int eflags)
>>       * verify that the proposed target relations are valid and open their
>>       * indexes for insertion of new index entries.  Note we *must* set
>>       * estate->es_result_relation_info correctly while we initialize each
>> -     * sub-plan; ExecContextForcesOids depends on that!
>> +     * sub-plan; external modules such as FDWs may depend on that.
>>
> 
> I think it will be better to include postgres_fdw in the comment in
> some way so that if someone wants a concrete example, there is
> something to refer to.

Maybe a good idea, but this will be the first time to mention postgres_fdw
in the core source code.  If you think that's OK, how about the attached?

Thanks,
Amit

Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> On 2019/05/30 18:51, Amit Kapila wrote:
>> I think it will be better to include postgres_fdw in the comment in
>> some way so that if someone wants a concrete example, there is
>> something to refer to.

> Maybe a good idea, but this will be the first time to mention postgres_fdw
> in the core source code.  If you think that's OK, how about the attached?

This wording seems fine to me.

Now that we've beat that item into the ground ... there were a bunch of
other tweaks suggested in Alexander's initial email.  Amit (K), were you
going to review/commit those?

            regards, tom lane

On Mon, Jun 3, 2019 at 10:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> > On 2019/05/30 18:51, Amit Kapila wrote:
> >> I think it will be better to include postgres_fdw in the comment in
> >> some way so that if someone wants a concrete example, there is
> >> something to refer to.
>
> > Maybe a good idea, but this will be the first time to mention postgres_fdw
> > in the core source code.  If you think that's OK, how about the attached?
>
> This wording seems fine to me.
>
> Now that we've beat that item into the ground ... there were a bunch of
> other tweaks suggested in Alexander's initial email.  Amit (K), were you
> going to review/commit those?
>

Yes, I am already reviewing those.  I will post my comments today.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Hi Andres,

I have added you here as some of these are related to commits done by
you.  So need your opinion on the same.  I have mentioned where your
feedback will be helpful.

On Sun, May 26, 2019 at 2:20 AM Alexander Lakhin <exclusion@gmail.com> wrote:
>
> 6. ExecGetResultSlot - remove (not used since introduction in 1a0586de)
>

Yeah, I also think this is not required.  Andres, this API is not
defined.  Is it intended for some purpose?

> 8. heap_parallelscan_initialize -> remove the sentence (changed in c2fe139c)
>

The same check has been moved to table_block_parallelscan_initialize.
So, I think instead of removing the sentence you need to change the
function name in the comment.

> 10. HeapTupleSatisfiesSnapshot -> HeapTupleSatisfiesVisibility (an
> internal inconsistency)
>

  * This is an interface to HeapTupleSatisfiesVacuum that's callable via
- * HeapTupleSatisfiesSnapshot, so it can be used through a Snapshot.
+ * HeapTupleSatisfiesVisibility, so it can be used through a Snapshot.

I think now we don't need to write the second half of the comment ("so
it can be used through a Snapshot").  It makes more sense with
previous style API.

Another related point:
* HeapTupleSatisfiesNonVacuumable
 *
 * True if tuple might be visible to some
transaction; false if it's
 * surely dead to everyone, ie, vacuumable.
 *
 * See SNAPSHOT_TOAST's definition for the intended behaviour.

Here, I think instead of SNAPSHOT_TOAST, we should mention
SNAPSHOT_NON_VACUUMABLE.

Andres, do you have any comments on the proposed changes?

> 14. latestRemovedxids -> latestRemovedXids (an inconsistent case)

* Conjecture: if hitemid is dead then it had xids before the xids
  * marked on LP_NORMAL items. So we just ignore this item and move
  * onto the next, for the purposes of calculating
- * latestRemovedxids.
+ * latestRemovedXids.

I think it should be latestRemovedXid.

> 16. NextSampletuple -> NextSampleTuple (an inconsistent case)

I think this doesn't make much difference, but we can fix it so that
NextSampleTuple's occurrence can be found during grep.

> 20. RelationGetOidIndex ? just to remove the paragraph (orphaned after
> 578b2297)

- * This is exported separately because there are cases where we want to use
- * an index that will not be recognized by RelationGetOidIndex: TOAST tables
- * have indexes that are usable, but have multiple columns and are on
- * ordinary columns rather than a true OID column.  This code will work
- * anyway, so long as the OID is the index's first column.  The caller must
- * pass in the actual heap attnum of the OID column, however.
- *

Instead of removing the entire paragraph, how about changing it like
"This also handles the special cases where TOAST tables have indexes
that are usable, but have multiple columns and are on ordinary columns
rather than a true OID column.  This code will work anyway, so long as
the OID is the index's first column.  The caller must
pass in the actual heap attnum of the OID column, however."

Andres, any suggestions?

> 27. XactTopTransactionId -> XactTopFullTransactionId (an internal
> inconsistency)
>

- * XactTopTransactionId stores the XID of our toplevel transaction, which
+ * XactTopFullTransactionId stores the XID of our toplevel transaction, which
  * will be the same as TopTransactionState.transactionId in an ordinary

I think in the above sentence, now we need to use
TopTransactionState.fullTransactionId.

Note that I agree with your changes for the points where I have not
responded anything.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

On Tue, Jun 4, 2019 at 7:36 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> Hi Andres,
>
> I have added you here as some of these are related to commits done by
> you.  So need your opinion on the same.  I have mentioned where your
> feedback will be helpful.
>
> > 10. HeapTupleSatisfiesSnapshot -> HeapTupleSatisfiesVisibility (an
> > internal inconsistency)
> >
>
>   * This is an interface to HeapTupleSatisfiesVacuum that's callable via
> - * HeapTupleSatisfiesSnapshot, so it can be used through a Snapshot.
> + * HeapTupleSatisfiesVisibility, so it can be used through a Snapshot.
>
> I think now we don't need to write the second half of the comment ("so
> it can be used through a Snapshot").  It makes more sense with
> previous style API.
>
> Another related point:
> * HeapTupleSatisfiesNonVacuumable
>  *
>  * True if tuple might be visible to some
> transaction; false if it's
>  * surely dead to everyone, ie, vacuumable.
>  *
>  * See SNAPSHOT_TOAST's definition for the intended behaviour.
>
> Here, I think instead of SNAPSHOT_TOAST, we should mention
> SNAPSHOT_NON_VACUUMABLE.
>
> Andres, do you have any comments on the proposed changes?
>
>
> > 20. RelationGetOidIndex ? just to remove the paragraph (orphaned after
> > 578b2297)
>
> - * This is exported separately because there are cases where we want to use
> - * an index that will not be recognized by RelationGetOidIndex: TOAST tables
> - * have indexes that are usable, but have multiple columns and are on
> - * ordinary columns rather than a true OID column.  This code will work
> - * anyway, so long as the OID is the index's first column.  The caller must
> - * pass in the actual heap attnum of the OID column, however.
> - *
>
> Instead of removing the entire paragraph, how about changing it like
> "This also handles the special cases where TOAST tables have indexes
> that are usable, but have multiple columns and are on ordinary columns
> rather than a true OID column.  This code will work anyway, so long as
> the OID is the index's first column.  The caller must
> pass in the actual heap attnum of the OID column, however."
>
> Andres, any suggestions?
>

Leaving the changes related to the above two points, I have combined
all the changes and fixed the things as per my review in the attached
patch.  Alexander, see if you can verify once the combined patch.  I
am planning to commit the attached by tomorrow and then we can deal
with the remaining two.  However, in the meantime, if Andres shared
his views on the above two points, then we can include the changes
corresponding to them as well.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

Hello Amit,
07.06.2019 7:30, Amit Kapila wrote:
> Leaving the changes related to the above two points, I have combined
> all the changes and fixed the things as per my review in the attached
> patch.  Alexander, see if you can verify once the combined patch.  I
> am planning to commit the attached by tomorrow and then we can deal
> with the remaining two.  However, in the meantime, if Andres shared
> his views on the above two points, then we can include the changes
> corresponding to them as well.
Amit, I agree with all of your changes. All I could is to move a dot:
.. (see contrib/postgres_fdw/postgres_fdw.c: postgresBeginDirectModify()
as one example).

Best regards,
Alexander

On Mon, Jun 3, 2019 at 10:56 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <Langote_Amit_f8@lab.ntt.co.jp> writes:
> > On 2019/05/30 18:51, Amit Kapila wrote:
> >> I think it will be better to include postgres_fdw in the comment in
> >> some way so that if someone wants a concrete example, there is
> >> something to refer to.
>
> > Maybe a good idea, but this will be the first time to mention postgres_fdw
> > in the core source code.  If you think that's OK, how about the attached?
>
> This wording seems fine to me.
>
> Now that we've beat that item into the ground ... there were a bunch of
> other tweaks suggested in Alexander's initial email.  Amit (K), were you
> going to review/commit those?
>

Pushed most of the changes except for two (point no. 10 and point no.
20) about which it is better if someone else can also comment.  I have
provided suggestions about those in my review email [1].   See, if you
have any comments on those.

[1] - https://www.postgresql.org/message-id/CAA4eK1J9_gdV22dRg-KaH_tnA1bXOUgLWCoJQikmPVyRbMHboA%40mail.gmail.com

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com