Re: Transactions involving multiple postgres foreign servers, take 2 - Mailing list pgsql-hackers

On Fri, Jan 15, 2021 at 7:45 AM Zhihong Yu <zyu@yugabyte.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/