On Wed, Mar 17, 2021 at 6:03 PM Zhihong Yu <zyu@yugabyte.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
wouldbecome 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.
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
