Re: Transactions involving multiple postgres foreign servers, take 2

On Fri, May 15, 2020 at 9:59 AM Masahiko Sawada <
masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:

> On Fri, 15 May 2020 at 13:26, Muhammad Usama <m(dot)usama(at)gmail(dot)com> wrote:
> >
> >
> >
> > On Fri, May 15, 2020 at 7:20 AM Masahiko Sawada <
> masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
> >>
> >> On Fri, 15 May 2020 at 03:08, Muhammad Usama <m(dot)usama(at)gmail(dot)com> wrote:
> >> >
> >> >
> >> > Hi Sawada,
> >> >
> >> > I have just done some review and testing of the patches and have
> >> > a couple of comments.
> >>
> >> Thank you for reviewing!
> >>
> >> >
> >> > 1- IMHO the PREPARE TRANSACTION should always use 2PC even
> >> > when the transaction has operated on a single foreign server
> regardless
> >> > of foreign_twophase_commit setting, and throw an error otherwise when
> >> > 2PC is not available on any of the data-modified servers.
> >> >
> >> > For example, consider the case
> >> >
> >> > BEGIN;
> >> > INSERT INTO ft_2pc_1 VALUES(1);
> >> > PREPARE TRANSACTION 'global_x1';
> >> >
> >> > Here since we are preparing the local transaction so we should also
> prepare
> >> > the transaction on the foreign server even if the transaction has
> modified only
> >> > one foreign table.
> >> >
> >> > What do you think?
> >>
> >> Good catch and I agree with you. The transaction should fail if it
> >> opened a transaction on a 2pc-no-support server regardless of
> >> foreign_twophase_commit. And I think we should prepare a transaction
> >> on a foreign server even if it didn't modify any data on that.
> >>
> >> >
> >> > Also without this change, the above test case produces an assertion
> failure
> >> > with your patches.
> >> >
> >> > 2- when deciding if the two-phase commit is required or not in
> >> > FOREIGN_TWOPHASE_COMMIT_PREFER mode we should use
> >> > 2PC when we have at least one server capable of doing that.
> >> >
> >> > i.e
> >> >
> >> > For FOREIGN_TWOPHASE_COMMIT_PREFER case in
> >> > checkForeignTwophaseCommitRequired() function I think
> >> > the condition should be
> >> >
> >> > need_twophase_commit = (nserverstwophase >= 1);
> >> > instead of
> >> > need_twophase_commit = (nserverstwophase >= 2);
> >> >
> >>
> >> Hmm I might be missing your point but it seems to me that you want to
> >> use two-phase commit even in the case where a transaction modified
> >> data on only one server. Can't we commit distributed transaction
> >> atomically even using one-phase commit in that case?
> >>
> >
> > I think you are confusing between nserverstwophase and nserverswritten.
> >
> > need_twophase_commit = (nserverstwophase >= 1) would mean
> > use two-phase commit if at least one server exists in the list that is
> > capable of doing 2PC
> >
> > For the case when the transaction modified data on only one server we
> > already exits the function indicating no two-phase required
> >
> > if (nserverswritten <= 1)
> > return false;
> >
>
> Thank you for your explanation. If the transaction modified two
> servers that don't' support 2pc and one server that supports 2pc I
> think we don't want to use 2pc even in 'prefer' case. Because even if
> we use 2pc in that case, it's still possible to have the atomic commit
> problem. For example, if we failed to commit a transaction after
> committing other transactions on the server that doesn't support 2pc
> we cannot rollback the already-committed transaction.
>

Yes, that is true, And I think the 'prefer' mode will always have a corner
case
no matter what. But the thing is we can reduce the probability of hitting
an atomic commit problem by ensuring to use 2PC whenever possible.

For instance as in your example scenario where a transaction modified
two servers that don't support 2PC and one server that supports it. let us
analyze both scenarios.

If we use 2PC on the server that supports it then the probability of hitting
a problem would be 1/3 = 0.33. because there is only one corner case
scenario in that case. which would be if we fail to commit the third server
As the first server (2PC supported one) would be using prepared
transactions so no problem there. The second server (NON-2PC support)
if failed to commit then, still no problem as we can rollback the prepared
transaction on the first server. The only issue would happen when we fail
to commit on the third server because we have already committed
on the second server and there is no way to undo that.

Now consider the other possibility if we do not use the 2PC in that
case (as you mentioned), then the probability of hitting the problem
would be 2/3 = 0.66. because now commit failure on either second or
third server will land us in an atomic-commit-problem.

So, INMO using the 2PC whenever available with 'prefer' mode
should be the way to go.

> On the other hand, in 'prefer' case, if the transaction also modified
> the local data, we need to use 2pc even if it modified data on only
> one foreign server that supports 2pc. But the current code doesn't
> work fine in that case for now. Probably we also need the following
> change:
>
> @@ -540,7 +540,10 @@ checkForeignTwophaseCommitRequired(void)
>
> /* Did we modify the local non-temporary data? */
> if ((MyXactFlags & XACT_FLAGS_WROTENONTEMPREL) != 0)
> + {
> nserverswritten++;
> + nserverstwophase++;
> + }
>
>
I agree with the part that if the transaction also modifies the local data
then the 2PC should be used.
Though the change you suggested [+ nserverstwophase++;]
would server the purpose and deliver the same results but I think a
better way would be to change need_twophase_commit condition for
prefer mode.

* In 'prefer' case, we prepare transactions on only servers that
* capable of two-phase commit.
*/
- need_twophase_commit = (nserverstwophase >= 2);
+ need_twophase_commit = (nserverstwophase >= 1);
}

The reason I am saying that is. Currently, we do not use 2PC on the local
server
in case of distributed transactions, so we should also not count the local
server
as one (servers that would be performing the 2PC).
Also I feel the change need_twophase_commit = (nserverstwophase >= 1)
looks more in line with the definition of our 'prefer' mode algorithm.

Do you see an issue with this change?

> Regards,
>
> --
> Masahiko Sawada http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Regards,
...
Muhammad Usama
Highgo Software (Canada/China/Pakistan)
URL : http://www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC