Re: Transactions involving multiple postgres foreign servers, take 2

On 2021/01/18 14:54, Masahiko Sawada 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.

You fixed some issues. But maybe you forgot to attach the latest patches?

I'm reading 0001 and 0002 patches to pick up the changes for postgres_fdw that worth applying independent from 2PC feature. If there are such changes, IMO we can apply them in advance, and which would make the patches simpler.

+ if (PQresultStatus(res) != PGRES_COMMAND_OK)
+ ereport(ERROR, (errmsg("could not commit transaction on server %s",
+ frstate->server->servername)));

You changed the code this way because you want to include the server name in the error message? I agree that it's helpful to report also the server name that caused an error. OTOH, since this change gets rid of call to pgfdw_rerport_error() for the returned PGresult, the reported error message contains less information. If this understanding is right, I don't think that this change is an improvement.

Instead, if the server name should be included in the error message, pgfdw_report_error() should be changed so that it also reports the server name? If we do that, the server name is reported not only when COMMIT fails but also when other commands fail.

Of course, if this change is not essential, we can skip doing this in the first version.

- /*
- * Regardless of the event type, we can now mark ourselves as out of the
- * transaction. (Note: if we are here during PRE_COMMIT or PRE_PREPARE,
- * this saves a useless scan of the hashtable during COMMIT or PREPARE.)
- */
- xact_got_connection = false;

With this change, xact_got_connection seems to never be set to false. Doesn't this break pgfdw_subxact_callback() using xact_got_connection?

+ /* Also reset cursor numbering for next transaction */
+ cursor_number = 0;

Originally this variable is reset to 0 once per transaction end. But with the patch, it's reset to 0 every time when a foreign transaction ends at each connection. This change would be harmless fortunately in practice, but seems not right theoretically.

This makes me wonder if new FDW API is not good at handling the case where some operations need to be performed once per transaction end.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION