Re: Transactions involving multiple postgres foreign servers, take 2

Hi,
For v35-0007-Prepare-foreign-transactions-at-commit-time.patch :

With this commit, the foreign server modified within the transaction marked
as 'modified'.

transaction marked -> transaction is marked

+#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.

+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 would become if
(!ServerSupportTwophaseCommit(fdw_part) && nserverswritten > 1).
have_no_twophase is no longer needed.

Cheers

On Tue, Mar 16, 2021 at 8:04 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
wrote:

> On Mon, Mar 15, 2021 at 3:55 AM Ibrar Ahmed <ibrar(dot)ahmad(at)gmail(dot)com> wrote:
> >
> >
> >
> > On Thu, Feb 11, 2021 at 6:25 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
> wrote:
> >>
> >> On Fri, Feb 5, 2021 at 2:45 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
> wrote:
> >> >
> >> > On Tue, Feb 2, 2021 at 5:18 PM Fujii Masao <
> masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> >> > >
> >> > >
> >> > >
> >> > > On 2021/01/27 14:08, Masahiko Sawada wrote:
> >> > > > On Wed, Jan 27, 2021 at 10:29 AM Fujii Masao
> >> > > > <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
> >> > > >>
> >> > > >>
> >> > > >> You fixed some issues. But maybe you forgot to attach the latest
> patches?
> >> > > >
> >> > > > Yes, I've attached the updated patches.
> >> > >
> >> > > Thanks for updating the patch! I tried to review 0001 and 0002 as
> the self-contained change.
> >> > >
> >> > > + * An FDW that implements both commit and rollback APIs can
> request to register
> >> > > + * the foreign transaction by FdwXactRegisterXact() to participate
> it to a
> >> > > + * group of distributed tranasction. The registered foreign
> transactions are
> >> > > + * identified by OIDs of server and user.
> >> > >
> >> > > I'm afraid that the combination of OIDs of server and user is not
> unique. IOW, more than one foreign transactions can have the same
> combination of OIDs of server and user. For example, the following two
> SELECT queries start the different foreign transactions but their user OID
> is the same. OID of user mapping should be used instead of OID of user?
> >> > >
> >> > > CREATE SERVER loopback FOREIGN DATA WRAPPER postgres_fdw;
> >> > > CREATE USER MAPPING FOR postgres SERVER loopback OPTIONS (user
> 'postgres');
> >> > > CREATE USER MAPPING FOR public SERVER loopback OPTIONS (user
> 'postgres');
> >> > > CREATE TABLE t(i int);
> >> > > CREATE FOREIGN TABLE ft(i int) SERVER loopback OPTIONS
> (table_name 't');
> >> > > BEGIN;
> >> > > SELECT * FROM ft;
> >> > > DROP USER MAPPING FOR postgres SERVER loopback ;
> >> > > SELECT * FROM ft;
> >> > > COMMIT;
> >> >
> >> > Good catch. I've considered using user mapping OID or a pair of user
> >> > mapping OID and server OID as a key of foreign transactions but I
> >> > think it also has a problem if an FDW caches the connection by pair of
> >> > server OID and user OID whereas the core identifies them by user
> >> > mapping OID. For instance, mysql_fdw manages connections by pair of
> >> > server OID and user OID.
> >> >
> >> > For example, let's consider the following execution:
> >> >
> >> > BEGIN;
> >> > SET ROLE user_A;
> >> > INSERT INTO ft1 VALUES (1);
> >> > SET ROLE user_B;
> >> > INSERT INTO ft1 VALUES (1);
> >> > COMMIT;
> >> >
> >> > Suppose that an FDW identifies the connections by {server OID, user
> >> > OID} and the core GTM identifies the transactions by user mapping OID,
> >> > and user_A and user_B use the public user mapping to connect server_X.
> >> > In the FDW, there are two connections identified by {user_A, sever_X}
> >> > and {user_B, server_X} respectively, and therefore opens two
> >> > transactions on each connection, while GTM has only one FdwXact entry
> >> > because the two connections refer to the same user mapping OID. As a
> >> > result, at the end of the transaction, GTM ends only one foreign
> >> > transaction, leaving another one.
> >> >
> >> > Using user mapping OID seems natural to me but I'm concerned that
> >> > changing role in the middle of transaction is likely to happen than
> >> > dropping the public user mapping but not sure. We would need to find
> >> > more better way.
> >>
> >> After more thought, I'm inclined to think it's better to identify
> >> foreign transactions by user mapping OID. The main reason is, I think
> >> FDWs that manages connection caches by pair of user OID and server OID
> >> potentially has a problem with the scenario Fujii-san mentioned. If an
> >> FDW has to use another user mapping (i.g., connection information) due
> >> to the currently used user mapping being removed, it would have to
> >> disconnect the previous connection because it has to use the same
> >> connection cache. But at that time it doesn't know the transaction
> >> will be committed or aborted.
> >>
> >> Also, such FDW has the same problem that postgres_fdw used to have; a
> >> backend establishes multiple connections with the same connection
> >> information if multiple local users use the public user mapping. Even
> >> from the perspective of foreign transaction management, it more makes
> >> sense that foreign transactions correspond to the connections to
> >> foreign servers, not to the local connection information.
> >>
> >> I can see that some FDW implementations such as mysql_fdw and
> >> firebird_fdw identify connections by pair of server OID and user OID
> >> but I think this is because they consulted to old postgres_fdw code. I
> >> suspect that there is no use case where FDW needs to identify
> >> connections in that way. If the core GTM identifies them by user
> >> mapping OID, we could enforce those FDWs to change their way but I
> >> think that change would be the right improvement.
> >>
> >> Regards,
> >>
> >> --
> >> Masahiko Sawada
> >> EDB: https://www.enterprisedb.com/
> >>
> >>
> >
> > Regression is failing, can you please take a look.
>
> Thank you!
>
> I've attached the updated version patch set.
>
> Regards,
>
> --
> Masahiko Sawada
> EDB: https://www.enterprisedb.com/
>