Re: bug in update tuple routing with foreign partitions

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

Great!

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

>> Notes:
>>
>> * I kept all the changes in the previous patch, because otherwise
>> postgres_fdw would fail to release resources for a foreign-insert
>> operation created by postgresBeginForeignInsert() for a tuple-routable
>> foreign table (ie, a foreign-table subplan resultrel that has been updated
>> already) during postgresEndForeignInsert().
>
> Hmm are you saying that the cases for which we'll still allow tuple
> routing (foreign table receiving moved-in rows has already been updated),
> there will be two fmstates to be released -- the original fmstate
> (UPDATE's) and aux_fmstate (INSERT's)?

Yeah, but I noticed that that explanation was not correct. (I think I
was really in hurry.) See the correction in [1].

>> * I revised a comment according to your previous comment, though I changed
>> a state name: s/sub_fmstate/aux_fmstate/
>
> That seems fine to me.

Cool!

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

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

Thanks for the comments!

Best regards,
Etsuro Fujita

[1] https://www.postgresql.org/message-id/5CB93D3F.6050903%40lab.ntt.co.jp