Re: Transactions involving multiple postgres foreign servers, take 2

On Sat, Feb 22, 2020 at 7:15 AM Masahiko Sawada <
masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:

> On Wed, 19 Feb 2020 at 07:55, Masahiko Sawada
> <masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
> >
> > On Tue, 11 Feb 2020 at 12:42, amul sul <sulamul(at)gmail(dot)com> wrote:
> > >
> > > On Fri, Jan 24, 2020 at 11:31 AM Masahiko Sawada <
> masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
> > >>
> > >> On Fri, 6 Dec 2019 at 17:33, Kyotaro Horiguchi <
> horikyota(dot)ntt(at)gmail(dot)com> wrote:
> > >> >
> > >> > Hello.
> > >> >
> > >> > This is the reased (and a bit fixed) version of the patch. This
> > >> > applies on the master HEAD and passes all provided tests.
> > >> >
> > >> > I took over this work from Sawada-san. I'll begin with reviewing the
> > >> > current patch.
> > >> >
> > >>
> > >> The previous patch set is no longer applied cleanly to the current
> > >> HEAD. I've updated and slightly modified the codes.
> > >>
> > >> This patch set has been marked as Waiting on Author for a long time
> > >> but the correct status now is Needs Review. The patch actually was
> > >> updated and incorporated all review comments but they was not rebased
> > >> actively.
> > >>
> > >> The mail[1] I posted before would be helpful to understand the current
> > >> patch design and there are README in the patch and a wiki page[2].
> > >>
> > >> I've marked this as Needs Review.
> > >>
> > >
> > > Hi Sawada san,
> > >
> > > I just had a quick look to 0001 and 0002 patch here is the few
> suggestions.
> > >
> > > patch: v27-0001:
> > >
> > > Typo: s/non-temprary/non-temporary
> > > ----
> > >
> > > patch: v27-0002: (Note:The left-hand number is the line number in the
> v27-0002 patch):
> > >
> > > 138 +PostgreSQL's the global transaction manager (GTM), as a
> distributed transaction
> > > 139 +participant The registered foreign transactions are tracked
> until the end of
> > >
> > > Full stop "." is missing after "participant"
> > >
> > >
> > > 174 +API Contract With Transaction Management Callback Functions
> > >
> > > Can we just say "Transaction Management Callback Functions";
> > > TOBH, I am not sure that I understand this title.
> > >
> > >
> > > 203 +processing foreign transaction (i.g. preparing, committing or
> aborting) the
> > >
> > > Do you mean "i.e" instead of i.g. ?
> > >
> > >
> > > 269 + * RollbackForeignTransactionAPI. Registered participant servers
> are identified
> > >
> > > Add space before between RollbackForeignTransaction and API.
> > >
> > >
> > > 292 + * automatically so must be processed manually using by
> pg_resovle_fdwxact()
> > >
> > > Do you mean pg_resolve_foreign_xact() here?
> > >
> > >
> > > 320 + * the foreign transaction is authorized to update the fields
> from its own
> > > 321 + * one.
> > > 322 +
> > > 323 + * Therefore, before doing PREPARE, COMMIT PREPARED or ROLLBACK
> PREPARED a
> > >
> > > Please add asterisk '*' on line#322.
> > >
> > >
> > > 816 +static void
> > > 817 +FdwXactPrepareForeignTransactions(void)
> > > 818 +{
> > > 819 + ListCell *lcell;
> > >
> > > Let's have this variable name as "lc" like elsewhere.
> > >
> > >
> > > 1036 + ereport(ERROR, (errmsg("could not insert a foreign
> transaction entry"),
> > > 1037 + errdetail("duplicate entry with
> transaction id %u, serverid %u, userid %u",
> > > 1038 + xid, serverid, userid)));
> > > 1039 + }
> > >
> > > Incorrect formatting.
> > >
> > >
> > > 1166 +/*
> > > 1167 + * Return true and set FdwXactAtomicCommitReady to true if the
> current transaction
> > >
> > > Do you mean ForeignTwophaseCommitIsRequired instead of
> FdwXactAtomicCommitReady?
> > >
> > >
> > > 3529 +
> > > 3530 +/*
> > > 3531 + * FdwXactLauncherRegister
> > > 3532 + * Register a background worker running the foreign
> transaction
> > > 3533 + * launcher.
> > > 3534 + */
> > >
> > > This prolog style is not consistent with the other function in the
> file.
> > >
> > >
> > > And here are the few typos:
> > >
> > > s/conssitent/consistent
> > > s/consisnts/consist
> > > s/Foriegn/Foreign
> > > s/tranascation/transaction
> > > s/itselft/itself
> > > s/rolbacked/rollbacked
> > > s/trasaction/transaction
> > > s/transactio/transaction
> > > s/automically/automatically
> > > s/CommitForeignTransaciton/CommitForeignTransaction
> > > s/Similary/Similarly
> > > s/FDWACT_/FDWXACT_
> > > s/dink/disk
> > > s/requried/required
> > > s/trasactions/transactions
> > > s/prepread/prepared
> > > s/preapred/prepared
> > > s/beging/being
> > > s/gxact/xact
> > > s/in-dbout/in-doubt
> > > s/respecitively/respectively
> > > s/transction/transaction
> > > s/idenetifier/identifier
> > > s/identifer/identifier
> > > s/checkpoint'S/checkpoint's
> > > s/fo/of
> > > s/transcation/transaction
> > > s/trasanction/transaction
> > > s/non-temprary/non-temporary
> > > s/resovler_internal.h/resolver_internal.h
> > >
> > >
> >
> > Thank you for reviewing the patch! I've incorporated all comments in
> > local branch.
>
> Attached the updated version patch sets that incorporated review
> comments from Amul and Muhammad.
>
> Regards,
>
> --
> Masahiko Sawada http://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Hi Sawada San,

I have been further reviewing and testing the transaction involving
multiple server patches.
Overall the patches are working as expected bar a few important exceptions.
So as discussed over the call I have fixed the issues I found during the
testing
and also rebased the patches with the current head of the master branch.
So can you please have a look at the attached updated patches.

Below is the list of changes I have made on top of V18 patches.

1- In register_fdwxact(), As we are just storing the callback function
pointers from
FdwRoutine in fdw_part structure, So I think we can avoid calling
GetFdwRoutineByServerId() in TopMemoryContext.
So I have moved the MemoryContextSwitch to TopMemoryContext after the
GetFdwRoutineByServerId() call.

2- If PrepareForeignTransaction functionality is not present in some FDW
then
during the registration process we should only set the
XACT_FLAGS_FDWNOPREPARE
transaction flag if the modified flag is also set for that server. As for
the server that has
not done any data modification within the transaction we do not do
two-phase commit anyway.

3- I have moved the foreign_twophase_commit in sample file after
max_foreign_transaction_resolvers because the default value of
max_foreign_transaction_resolvers
is 0 and enabling the foreign_twophase_commit produces an error with default
configuration parameter positioning in postgresql.conf
Also, foreign_twophase_commit configuration was missing the comments
about allowed values in the sample config file.

4- Setting ForeignTwophaseCommitIsRequired in
is_foreign_twophase_commit_required()
function does not seem to be the correct place. The reason being, even when
*is_foreign_twophase_commit_required() *returns true after setting
ForeignTwophaseCommitIsRequired
to true, we could still end up not using the two-phase commit in the case
when some server does
not support two-phase commit and foreign_twophase_commit is set to
FOREIGN_TWOPHASE_COMMIT_PREFER
mode. So I have moved the ForeignTwophaseCommitIsRequired assignment to
PreCommit_FdwXacts()
function after doing the prepare transaction.

6- In prefer mode, we commit the transaction in single-phase if the server
does not support
the two-phase commit. But instead of doing the single-phase commit right
away,
IMHO the better way is to wait until all the two-phase transactions are
successfully prepared
on servers that support the two-phase. Since an error during a "PREPARE"
stage would
rollback the transaction and in that case, we would end up with committed
transactions on
the server that lacks the support of the two-phase commit.
So I have modified the flow a little bit and instead of doing a one-phase
commit right away
the servers that do not support a two-phase commit is added to another list
and that list is
processed after once we have successfully prepared all the transactions on
two-phase supported
foreign servers. Although this technique is also not bulletproof, still it
is better than doing
the one-phase commits before doing the PREPAREs.

Also, I think we can improve on this one by throwing an error even in PREFER
mode if there is more than one server that had data modified within the
transaction
and lacks the two-phase commit support.

7- Added a pfree() and list_free_deep() in PreCommit_FdwXacts() to reclaim
the
memory if fdw_part is removed from the list

8- The function FdwXactWaitToBeResolved() was bailing out as soon as it
finds
(FdwXactParticipants == NIL). The problem with that was in the case of
"COMMIT/ROLLBACK PREPARED" we always get FdwXactParticipants = NIL and
effectively the foreign prepared transactions(if any) associated with
locally
prepared transactions were never getting resolved automatically.

postgres=# BEGIN;
BEGIN
INSERT INTO test_local VALUES ( 2, 'TWO');
INSERT 0 1
INSERT INTO test_foreign_s1 VALUES ( 2, 'TWO');
INSERT 0 1
INSERT INTO test_foreign_s2 VALUES ( 2, 'TWO');
INSERT 0 1
postgres=*# PREPARE TRANSACTION 'local_prepared';
PREPARE TRANSACTION

postgres=# select * from pg_foreign_xacts ;
dbid | xid | serverid | userid | status | in_doubt | identifier

-------+-----+----------+--------+----------+----------+----------------------------
12929 | 515 | 16389 | 10 | prepared | f |
fx_1339567411_515_16389_10
12929 | 515 | 16391 | 10 | prepared | f |
fx_1963224020_515_16391_10
(2 rows)

-- Now commit the prepared transaction

postgres=# COMMIT PREPARED 'local_prepared';

COMMIT PREPARED

--Foreign prepared transactions associated with 'local_prepared' not
resolved

postgres=#
postgres=# select * from pg_foreign_xacts ;
dbid | xid | serverid | userid | status | in_doubt | identifier

-------+-----+----------+--------+----------+----------+----------------------------
12929 | 515 | 16389 | 10 | prepared | f |
fx_1339567411_515_16389_10
12929 | 515 | 16391 | 10 | prepared | f |
fx_1963224020_515_16391_10
(2 rows)

So to fix this in case of the two-phase transaction, the function checks
the existence
of associated foreign prepared transactions before bailing out.

9- In function XlogReadFdwXactData() XLogBeginRead call was missing before
XLogReadRecord()
that was causing the crash during recovery.

10- incorporated set_ps_display() signature change.

Best regards,

...
Muhammad Usama
HighGo Software (Canada/China/Pakistan)
URL : http://www.highgo.ca