Re: Transactions involving multiple postgres foreign servers, take 2

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?

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

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

> 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()?

>
> 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);
}
}

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

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.

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.

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.

Regards,

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