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