Re: bug in update tuple routing with foreign partitions

Fujita-san,

On 2019/04/22 20:00, Etsuro Fujita wrote:
> (2019/04/19 14:39), Etsuro Fujita wrote:
>> (2019/04/19 13:00), Amit Langote wrote:
>>> 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.

Ah, you're right. The case that would return wrong result, that is now
prevented per the latest patch, is also the case that would crash before.
So, it seems to OK to keep this commit this as one patch. Sorry for the
noise.

I read your commit message and it seems to sufficiently explain the issues
being fixed. Thanks for adding me as an author, but I think the latest
patch is mostly your work, so I'm happy to be listed as just a reviewer. :)

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

Thanks, looks fine.

> Other changes:
> * I updated the docs in postgres-fdw.sgml to mention the limitation.

Looks fine.

> * I did some cleanups for the regression tests.

Here too.

> Please find attached an updated version of the patch.

I don't have any more comments. Thanks for working on this.

Thanks,
Amit