On Wed, Jan 27, 2021 at 10:29 AM Fujii Masao
<masao.fujii@oss.nttdata.com> wrote:
>
>
> You fixed some issues. But maybe you forgot to attach the latest patches?
Yes, I've attached the updated 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.
Thank you for reviewing the patches!
>
> + 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
helpfulto 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
understandingis right, I don't think that this change is an improvement.
Right. It's better to use do_sql_command() instead.
> Instead, if the server name should be included in the error message, pgfdw_report_error() should be changed so that
italso reports the server name? If we do that, the server name is reported not only when COMMIT fails but also when
othercommands fail.
>
> Of course, if this change is not essential, we can skip doing this in the first version.
Yes, I think it's not essential for now. We can improve it later if we want.
>
> - /*
> - * 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()
usingxact_got_connection?
I think xact_got_connection is set to false in
pgfdw_cleanup_after_transaction() that is called at the end of each
foreign transaction (i.g., in postgresCommitForeignTransaction() and
postgresRollbackForeignTransaction()).
But as you're concerned below, it's reset for each foreign transaction
end rather than the parent's transaction end.
>
> + /* 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
aforeign transaction ends at each connection. This change would be harmless fortunately in practice, but seems not
righttheoretically.
>
> This makes me wonder if new FDW API is not good at handling the case where some operations need to be performed once
pertransaction end.
I think that the problem comes from the fact that FDW needs to use
both SubXactCallback and new FDW API.
If we want to perform some operations at the end of the top
transaction per FDW, not per foreign transaction, we will either still
need to use XactCallback or need to rethink the FDW API design. But
given that we call commit and rollback FDW API for only foreign
servers that actually started a transaction, I’m not sure if there are
such operations in practice. IIUC there is not at least from the
normal (not-sub) transaction termination perspective.
IIUC xact_got_transaction is used to skip iterating over all cached
connections to find open remote (sub) transactions. This is not
necessary anymore at least from the normal transaction termination
perspective. So maybe we can improve it so that it tracks whether any
of the cached connections opened a subtransaction. That is, we set it
true when we created a savepoint on any connections and set it false
at the end of pgfdw_subxact_callback() if we see that xact_depth of
all cached entry is less than or equal to 1 after iterating over all
entries.
Regarding cursor_number, it essentially needs to be unique at least
within a transaction so we can manage it per transaction or per
connection. But the current postgres_fdw rather ensure uniqueness
across all connections. So it seems to me that this can be fixed by
making individual connection have cursor_number and resetting it in
pgfdw_cleanup_after_transaction(). I think this can be in a separate
patch. Or it also could solve this problem that we terminate
subtransactions via a FDW API but I don't think it's a good idea.
What do you think?
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
