Re: bug in update tuple routing with foreign partitions

(2019/04/19 14:39), Etsuro Fujita wrote:
> (2019/04/19 13:00), Amit Langote wrote:
>> On 2019/04/18 22:10, Etsuro Fujita wrote:
>>>> On 2019/04/18 14:06, Amit Langote wrote:
>>>>> On 2019/04/17 21:49, Etsuro Fujita wrote:
>>>>>> So what I'm thinking is to throw an error for cases like this.
>>>>>> (Though, I
>>>>>> think we should keep to allow rows to be moved from local
>>>>>> partitions to a
>>>>>> foreign-table subplan targetrel that has been updated already.)
>>>>>
>>>>> Hmm, how would you distinguish (totally inside postgred_fdw I
>>>>> presume) the
>>>>> two cases?
>>>
>>> One thing I came up with to do that is this:
>>>
>>> @@ -1917,6 +1937,23 @@ postgresBeginForeignInsert(ModifyTableState
>>> *mtstate,
>>>
>>> initStringInfo(&sql);
>>>
>>> + /*
>>> + * If the foreign table is an UPDATE subplan resultrel that hasn't
>>> yet
>>> + * been updated, routing tuples to the table might yield incorrect
>>> + * results, because if routing tuples, routed tuples will be
>>> mistakenly
>>> + * read from the table and updated twice when updating the table
>>> --- it
>>> + * would be nice if we could handle this case; but for now, throw
>>> an error
>>> + * for safety.
>>> + */
>>> + if (plan&& plan->operation == CMD_UPDATE&&
>>> + (resultRelInfo->ri_usesFdwDirectModify ||
>>> + resultRelInfo->ri_FdwState)&&
>>> + resultRelInfo> mtstate->resultRelInfo +
>>> mtstate->mt_whichplan)
>>> + ereport(ERROR,
>>> + (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>>> + errmsg("cannot route tuples into foreign
>>> table to be updated \"%s\"",
>>> + RelationGetRelationName(rel))));
>>
>> Oh, I see.
>>
>> So IIUC, you're making postgresBeginForeignInsert() check two things:
>>
>> 1. Whether the foreign table it's about to begin inserting (moved) rows
>> into is a subplan result rel, checked by
>> (resultRelInfo->ri_usesFdwDirectModify || resultRelInfo->ri_FdwState)
>>
>> 2. Whether the foreign table it's about to begin inserting (moved) rows
>> into will be updated later, checked by (resultRelInfo>
>> mtstate->resultRelInfo + mtstate->mt_whichplan)
>>
>> This still allows a foreign table to receive rows moved from the local
>> partitions if it has already finished the UPDATE operation.
>>
>> Seems reasonable to me.

>>>> Forgot to say that since this is a separate issue from the original bug
>>>> report, maybe we can first finish fixing the latter. What to do you
>>>> think?
>>>
>>> Yeah, I think we can do that, but my favorite would be to fix the two
>>> issues in a single shot, because they seem to me rather close to each
>>> other. So I updated a new version in a single patch, which I'm
>>> attaching.
>>
>> I agree that we can move to fix the other issue right away as the fix you
>> outlined above seems reasonable, but I wonder if it wouldn't be better to
>> commit the two fixes separately? The two fixes, although small, are
>> somewhat complicated and combining them in a single commit might be
>> confusing. Also, a combined commit might make it harder for the release
>> note author to list down the exact set of problems being fixed.
>
> OK, I'll separate the patch into two.

After I tried to separate the patch a bit, I changed my mind: I think we 
should commit the two issues in a single patch, because the original 
issue that overriding fmstate for the UPDATE operation mistakenly by 
fmstate for the INSERT operation caused backend crash is fixed by what I 
proposed above.  So I add the commit message to the previous single 
patch, as you suggested.

>> Some mostly cosmetic comments on the code changes:
>>
>> * In the following comment:
>>
>> + /*
>> + * If the foreign table is an UPDATE subplan resultrel that hasn't yet
>> + * been updated, routing tuples to the table might yield incorrect
>> + * results, because if routing tuples, routed tuples will be mistakenly
>> + * read from the table and updated twice when updating the table --- it
>> + * would be nice if we could handle this case; but for now, throw an
>> error
>> + * for safety.
>> + */
>>
>> the part that start with "because if routing tuples..." reads a bit
>> unclear to me. How about writing this as:
>>
>> /*
>> * If the foreign table we are about to insert routed rows into is
>> * also an UPDATE result rel and the UPDATE hasn't been performed yet,
>> * proceeding with the INSERT will result in the later UPDATE
>> * incorrectly modifying those routed rows, so prevent the INSERT ---
>> * it would be nice if we could handle this case, but for now, throw
>> * an error for safety.
>> */
>
> I think that's better than mine; will use that wording.

Done.  I simplified your wording a little bit, though.

>> I see that in all the hunks involving some manipulation of aux_fmstate,
>> there's a comment explaining what it is, which seems a bit repetitive. I
>> can see more or less the same explanation in postgresExecForeignInsert(),
>> postgresBeginForeignInsert(), and postgresEndForeignInsert(). Maybe just
>> keep the description in postgresBeginForeignInsert as follows:
>>
>> @@ -1983,7 +2020,19 @@ postgresBeginForeignInsert(ModifyTableState
>> *mtstate,
>> retrieved_attrs != NIL,
>> retrieved_attrs);
>>
>> - resultRelInfo->ri_FdwState = fmstate;
>> + /*
>> + * If the given resultRelInfo already has PgFdwModifyState set, it means
>> + * the foreign table is an UPDATE subplan resultrel; in which case,
>> store
>> + * the resulting state into the aux_fmstate of the PgFdwModifyState.
>> + */
>>
>> and change the other sites to refer to postgresBeginForeingInsert for the
>> detailed explanation of what aux_fmstate is.
>
> Good idea; will do.

Done.

Other changes:
* I updated the docs in postgres-fdw.sgml to mention the limitation.
* I did some cleanups for the regression tests.

Please find attached an updated version of the patch.

Best regards,
Etsuro Fujita