Re: Transactions involving multiple postgres foreign servers, take 2

Hi, Masahiko-san:

bq. How about FdwXactRequestToLaunchResolver()?

Sounds good to me.

bq. But there is already a function named FdwXactExists()

Then we can leave the function name as it is.

Cheers

On Sun, Jan 17, 2021 at 9:55 PM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
wrote:

> On Fri, Jan 15, 2021 at 7:45 AM Zhihong Yu <zyu(at)yugabyte(dot)com> wrote:
> >
> > For v32-0002-postgres_fdw-supports-commit-and-rollback-APIs.patch :
> >
> > + entry->changing_xact_state = true;
> > ...
> > + entry->changing_xact_state = abort_cleanup_failure;
> >
> > I don't see return statement in between the two assignments. I wonder
> why entry->changing_xact_state is set to true, and later being assigned
> again.
>
> Because postgresRollbackForeignTransaction() can get called again in
> case where an error occurred during aborting and cleanup the
> transaction. For example, if an error occurred when executing ABORT
> TRANSACTION (pgfdw_get_cleanup_result() could emit an ERROR),
> postgresRollbackForeignTransaction() will get called again while
> entry->changing_xact_state is still true. Then the entry will be
> caught by the following condition and cleaned up:
>
> /*
> * If connection is before starting transaction or is already
> unsalvageable,
> * do only the cleanup and don't touch it further.
> */
> if (entry->changing_xact_state)
> {
> pgfdw_cleanup_after_transaction(entry);
> return;
> }
>
> >
> > For v32-0007-Introduce-foreign-transaction-launcher-and-resol.patch :
> >
> > bq. This commits introduces to new background processes: foreign
> >
> > commits introduces to new -> commit introduces two new
>
> Fixed.
>
> >
> > +FdwXactExistsXid(TransactionId xid)
> >
> > Since Xid is the parameter to this method, I think the Xid suffix can be
> dropped from the method name.
>
> But there is already a function named FdwXactExists()?
>
> bool
> FdwXactExists(Oid dbid, Oid serverid, Oid userid)
>
> As far as I read other code, we already have such functions that have
> the same functionality but have different arguments. For instance,
> SearchSysCacheExists() and SearchSysCacheExistsAttName(). So I think
> we can leave as it is but is it better to have like
> FdwXactCheckExistence() and FdwXactCheckExistenceByXid()?
>
> >
> > + * Portions Copyright (c) 2020, PostgreSQL Global Development Group
> >
> > Please correct year in the next patch set.
>
> Fixed.
>
> >
> > +FdwXactLauncherRequestToLaunch(void)
> >
> > Since the launcher's job is to 'launch', I think the Launcher can be
> omitted from the method name.
>
> Agreed. How about FdwXactRequestToLaunchResolver()?
>
> >
> > +/* Report shared memory space needed by FdwXactRsoverShmemInit */
> > +Size
> > +FdwXactRslvShmemSize(void)
> >
> > Are both Rsover and Rslv referring to resolver ? It would be better to
> use whole word which reduces confusion.
> > Plus, FdwXactRsoverShmemInit should be FdwXactRslvShmemInit (or
> FdwXactResolveShmemInit)
>
> Agreed. I realized that these functions are the launcher's function,
> not resolver's. So I'd change to FdwXactLauncherShmemSize() and
> FdwXactLauncherShmemInit() respectively.
>
> >
> > +fdwxact_launch_resolver(Oid dbid)
> >
> > The above method is not in camel case. It would be better if method
> names are consistent (in casing).
>
> Fixed.
>
> >
> > + errmsg("out of foreign transaction resolver slots"),
> > + errhint("You might need to increase
> max_foreign_transaction_resolvers.")));
> >
> > It would be nice to include the value of max_foreign_xact_resolvers
>
> I agree it would be nice but looking at other code we don't include
> the value in this kind of messages.
>
> >
> > For fdwxact_resolver_onexit():
> >
> > + LWLockAcquire(FdwXactLock, LW_EXCLUSIVE);
> > + fdwxact->locking_backend = InvalidBackendId;
> > + LWLockRelease(FdwXactLock);
> >
> > There is no call to method inside the for loop which may take time. I
> wonder if the lock can be obtained prior to the for loop and released
> coming out of the for loop.
>
> Agreed.
>
> >
> > +FXRslvLoop(void)
> >
> > Please use Resolver instead of Rslv
>
> Fixed.
>
> >
> > + FdwXactResolveFdwXacts(held_fdwxacts, nheld);
> >
> > Fdw and Xact are repeated twice each in the method name. Probably the
> method name can be made shorter.
>
> Fixed.
>
> Regards,
>
> --
> Masahiko Sawada
> EnterpriseDB: https://www.enterprisedb.com/
>