Re: Transactions involving multiple postgres foreign servers, take 2

On Mon, May 3, 2021 at 11:11 PM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:
>
>
>
> On Mon, May 3, 2021 at 5:25 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>>
>> On Sun, May 2, 2021 at 1:23 AM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:
>> >
>> >
>> >
>> > On Fri, Apr 30, 2021 at 9:09 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
>> >>
>> >> On Wed, Mar 17, 2021 at 6:03 PM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:
>> >> >
>> >> > Hi,
>> >> > For v35-0007-Prepare-foreign-transactions-at-commit-time.patch :
>> >>
>> >> Thank you for reviewing the patch!
>> >>
>> >> >
>> >> > With this commit, the foreign server modified within the transaction marked as 'modified'.
>> >> >
>> >> > transaction marked -> transaction is marked
>> >>
>> >> Will fix.
>> >>
>> >> >
>> >> > +#define IsForeignTwophaseCommitRequested() \
>> >> > + (foreign_twophase_commit > FOREIGN_TWOPHASE_COMMIT_DISABLED)
>> >> >
>> >> > Since the other enum is FOREIGN_TWOPHASE_COMMIT_REQUIRED, I think the macro should be named: IsForeignTwophaseCommitRequired.
>> >>
>> >> But even if foreign_twophase_commit is
>> >> FOREIGN_TWOPHASE_COMMIT_REQUIRED, the two-phase commit is not used if
>> >> there is only one modified server, right? It seems the name
>> >> IsForeignTwophaseCommitRequested is fine.
>> >>
>> >> >
>> >> > +static bool
>> >> > +checkForeignTwophaseCommitRequired(bool local_modified)
>> >> >
>> >> > + if (!ServerSupportTwophaseCommit(fdw_part))
>> >> > + have_no_twophase = true;
>> >> > ...
>> >> > + if (have_no_twophase)
>> >> > + ereport(ERROR,
>> >> >
>> >> > It seems the error case should be reported within the loop. This way, we don't need to iterate the other participant(s).
>> >> > Accordingly, nserverswritten should be incremented for local server prior to the loop. The condition in the loop would become if (!ServerSupportTwophaseCommit(fdw_part) && nserverswritten > 1).
>> >> > have_no_twophase is no longer needed.
>> >>
>> >> Hmm, I think If we process one 2pc-non-capable server first and then
>> >> process another one 2pc-capable server, we should raise an error but
>> >> cannot detect that.
>> >
>> >
>> > Then the check would stay as what you have in the patch:
>> >
>> > if (!ServerSupportTwophaseCommit(fdw_part))
>> >
>> > When the non-2pc-capable server is encountered, we would report the error in place (following the ServerSupportTwophaseCommit check) and come out of the loop.
>> > have_no_twophase can be dropped.
>>
>> But if we processed only one non-2pc-capable server, we would raise an
>> error but should not in that case.
>>
>> On second thought, I think we can track how many servers are modified
>> or not capable of 2PC during registration and unr-egistration. Then we
>> can consider both 2PC is required and there is non-2pc-capable server
>> is involved without looking through all participants. Thoughts?
>
>
> That is something worth trying.
>

I've attached the updated patches that incorporated comments from
Zhihong and Ikeda-san.

Regards,

--
Masahiko Sawada
EDB: https://www.enterprisedb.com/