Re: [HACKERS] Update comments in nodeModifyTable.c

On 2017/08/01 1:03, Robert Haas wrote:
> On Fri, Jul 28, 2017 at 8:12 AM, Etsuro Fujita
> <fujita.etsuro@lab.ntt.co.jp> wrote:
>> On 2017/07/26 22:39, Robert Haas wrote:
>>> So the first part of the change weakens the assertion that a 'ctid' or
>>> 'wholerow' attribute will always be present by saying that an FDW may
>>> instead have other attributes sufficient to identify the row.
>>
>> That's right.
>>
>>> But
>>> then the additional sentence says that there will be a 'wholerow'
>>> attribute after all.  So this whole change seems to me to be going
>>> around in a circle.
>>
>> What I mean by the additional one is: if the result table that is a foreign
>> table has a row-level UPDATE/DELETE trigger, a 'wholerow' will also be
>> present.  So, if the result table didn't have the trigger, there wouldn't be
>> 'whole-row', so in that case it could be possible that there would be only
>> attributes other than 'ctid' and 'wholerow'.
> 
> I think maybe something like this would be clearer, then:
> 
>       /*
>        * Initialize the junk filter(s) if needed.  INSERT queries need a filter
>        * if there are any junk attrs in the tlist.  UPDATE and DELETE always
> -     * need a filter, since there's always a junk 'ctid' or 'wholerow'
> -     * attribute present --- no need to look first.
> +     * need a filter, since there's always at least one junk attribute present
> +     * --- no need to look first.  Typically, this will be a 'ctid'
> +     * attribute, but in the case of a foreign data wrapper it might be a
> +     * 'wholerow' attribute or some other set of junk attributes sufficient to
> +     * identify the remote row.
>        *
>        * If there are multiple result relations, each one needs its own junk
>        * filter.  Note multiple rels are only possible for UPDATE/DELETE, so we

Maybe I'm missing something, but I'm not sure that's a good idea because 
the change says like we might have 'wholerow' only for the FDW case, but 
that isn't correct because we would have 'wholerow' for a view as well. 
ISTM that views should be one of the typical cases, so I'd like to 
propose to modify the sentence starting with 'Typically' to something 
like this: "Typically, this will be a 'ctid' or 'wholerow' attribute, 
but in the case of a foreign data wrapper it might be a set of junk 
attributes sufficient to identify the remote row."  What do you think 
about that?

Best regards,
Etsuro Fujita