Re: Transactions involving multiple postgres foreign servers, take 2

On Wed, Apr 8, 2020 at 11:16 AM Masahiko Sawada <
masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:

> On Fri, 27 Mar 2020 at 22:06, Muhammad Usama <m(dot)usama(at)gmail(dot)com> wrote:
> >
> > 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.
>
> Thank you for reviewing and updating the patch!
>
> >
> > 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.
>
> Agreed.
>
> >
> >
> > 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.
>
> Agreed.
>
> >
> > 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.
>
> Sounds good. Agreed.
>
> >
> > 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.
>
> Agreed.
>
> >
> > 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.
>
> When an error occurred before the local commit, a 2pc-unsupported
> server could be rolled back or committed depending on the error
> timing. On the other hand all 2pc-supported servers are always rolled
> back when an error occurred before the local commit. Therefore even if
> we change the order of COMMIT and PREPARE it is still possible that we
> will end up committing the part of 2pc-unsupported servers while
> rolling back others including 2pc-supported servers.
>
> I guess the motivation of your change is that since errors are likely
> to happen during executing PREPARE on foreign servers, we can minimize
> the possibility of rolling back 2pc-unsupported servers by deferring
> the commit of 2pc-unsupported server as much as possible. Is that
> right?
>

Yes, that is correct. The idea of doing the COMMIT on NON-2pc-supported
servers
after all the PREPAREs are successful is to minimize the chances of partial
commits.
And as you mentioned there will still be chances of getting a partial
commit even with
this approach but the probability of that would be less than what it is
with the
current sequence.

>
> > 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.
>
> Hmm the current logic seems complex. Maybe we can just reverse the
> order of COMMIT and PREPARE; do PREPARE on all 2pc-supported and
> modified servers first and then do COMMIT on others?
>

Agreed, seems reasonable.

>
> >
> > 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.
> >
>
> IIUC the concept of PREFER mode is that the transaction uses 2pc only
> for 2pc-supported servers. IOW, even if the transaction modifies on a
> 2pc-unsupported server we can proceed with the commit if in PREFER
> mode, which cannot if in REQUIRED mode. What is the motivation of your
> above idea?
>

I was thinking that we could change the behavior of PREFER mode such that
we only allow
to COMMIT the transaction if the transaction needs to do a single-phase
commit on one
server only. That way we can ensure that we would never end up with partial
commit.

One Idea in this regards would be to switch the local transaction to commit
using 2pc
if there is a total of only one foreign server that does not support the
2pc in the transaction,
ensuring that 1-pc commit servers should always be less than or equal to 1.
and if there are more
than one foreign server requires 1-pc then we just throw an error.

However having said that, I am not 100% sure if its a good or an acceptable
Idea, and
I am okay with continuing with the current behavior of PREFER mode if we
put it in the
document that this mode can cause a partial commit.

> > 7- Added a pfree() and list_free_deep() in PreCommit_FdwXacts() to
> reclaim the
> > memory if fdw_part is removed from the list
>
> I think at the end of the transaction we free entries of
> FdwXactParticipants list and set FdwXactParticipants to NIL. Why do we
> need to do that in PreCommit_FdwXacts()?
>

Correct me if I am wrong, The fdw_part structures are created in
TopMemoryContext
and if that fdw_part structure is removed from the list at pre_commit stage
(because we did 1-PC COMMIT on it) then it would leak memory.

> >
> > 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.
> >
>
> Good catch. But looking at your change, we should not accept the case
> where FdwXactParticipants == NULL but TwoPhaseExists(wait_xid) ==
> false.
>
> if (FdwXactParticipants == NIL)
> {
> /*
> * If we are here because of COMMIT/ROLLBACK PREPARED then
> the
> * FdwXactParticipants list would be empty. So we need to
> * see if there are any foreign prepared transactions exists
> * for this prepared transaction
> */
> if (TwoPhaseExists(wait_xid))
> {
> List *foreign_trans = NIL;
>
> foreign_trans = get_fdwxacts(MyDatabaseId,
> wait_xid, InvalidOid, InvalidOid,
> false, false, true);
>
> if (foreign_trans == NIL)
> return;
> list_free(foreign_trans);
> }
> }
>
>
Sorry my bad, its a mistake on my part. we should just return from the
function when
FdwXactParticipants == NULL but TwoPhaseExists(wait_xid) == false.

if (TwoPhaseExists(wait_xid))
{
List *foreign_trans = NIL;
foreign_trans = get_fdwxacts(MyDatabaseId, wait_xid,
InvalidOid, InvalidOid,
false, false, true);

if (foreign_trans == NIL)
return;
list_free(foreign_trans);
}
else
return;

> > 9- In function XlogReadFdwXactData() XLogBeginRead call was missing
> before XLogReadRecord()
> > that was causing the crash during recovery.
>
> Agreed.
>
> >
> > 10- incorporated set_ps_display() signature change.
>
> Thanks.
>
> Regarding other changes you did in v19 patch, I have some comments:
>
> 1.
> + ereport(LOG,
> + (errmsg("trying to %s the foreign transaction
> associated with transaction %u on server %u",
> + fdwxact->status ==
> FDWXACT_STATUS_COMMITTING?"COMMIT":"ABORT",
> + fdwxact->local_xid,
> fdwxact->serverid)));
> +
>
> Why do we need to emit LOG message in pg_resolve_foreign_xact() SQL
> function?
>

That change was not intended to get into the patch file. I had done it
during testing to
quickly get info on which way the transaction is going to be resolved.

> 2.
> diff --git a/src/bin/pg_waldump/fdwxactdesc.c
> b/src/bin/pg_waldump/fdwxactdesc.c
> deleted file mode 120000
> index ce8c21880c..0000000000
> --- a/src/bin/pg_waldump/fdwxactdesc.c
> +++ /dev/null
> @@ -1 +0,0 @@
> -../../../src/backend/access/rmgrdesc/fdwxactdesc.c
> \ No newline at end of file
> diff --git a/src/bin/pg_waldump/fdwxactdesc.c
> b/src/bin/pg_waldump/fdwxactdesc.c
> new file mode 100644
> index 0000000000..ce8c21880c
> --- /dev/null
> +++ b/src/bin/pg_waldump/fdwxactdesc.c
> @@ -0,0 +1 @@
> +../../../src/backend/access/rmgrdesc/fdwxactdesc.c
>
> We need to remove src/bin/pg_waldump/fdwxactdesc.c from the patch.
>

Again sorry! that was an oversight on my part.

> 3.
> --- a/doc/src/sgml/monitoring.sgml
> +++ b/doc/src/sgml/monitoring.sgml
> @@ -1526,14 +1526,14 @@ postgres 27093 0.0 0.0 30096 2752 ?
> Ss 11:34 0:00 postgres: ser
> <entry><literal>SafeSnapshot</literal></entry>
> <entry>Waiting for a snapshot for a <literal>READ ONLY
> DEFERRABLE</literal> transaction.</entry>
> </row>
> - <row>
> - <entry><literal>SyncRep</literal></entry>
> - <entry>Waiting for confirmation from remote server during
> synchronous replication.</entry>
> - </row>
> <row>
> <entry><literal>FdwXactResolution</literal></entry>
> <entry>Waiting for all foreign transaction participants to
> be resolved during atomic commit among foreign servers.</entry>
> </row>
> + <row>
> + <entry><literal>SyncRep</literal></entry>
> + <entry>Waiting for confirmation from remote server during
> synchronous replication.</entry>
> + </row>
> <row>
> <entry morerows="4"><literal>Timeout</literal></entry>
> <entry><literal>BaseBackupThrottle</literal></entry>
>
> We need to move the entry of FdwXactResolution to right before
> Hash/Batch/Allocating for alphabetical order.
>

Agreed!

>
> I've incorporated your changes I agreed with to my local branch and
> will incorporate other changes after discussion. I'll also do more
> test and self-review and will submit the latest version patch.
>
>
Meanwhile, I found a couple of more small issues, One is the break
statement missing
i n pgstat_get_wait_ipc() and secondly fdwxact_relaunch_resolvers()
could return un-initialized value.
I am attaching a small patch for these changes that can be applied on top
of existing
patches.

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

Best Regards,
Muhammad Usama
Highgo Software
URL : http://www.highgo.ca