On 2020/09/08 19:36, Alexey Kondratov wrote:
On 2020-09-08 05:49, Fujii Masao wrote:
On 2020/09/05 3:31, Alexey Kondratov wrote:
Attached is a patch, which implements a plain 2PC in the postgres_fdw and adds
a GUC 'postgres_fdw.use_twophase'. Also it solves these errors handling issues
above and tries to add proper comments everywhere. I think, that 0003 should be
rebased on the top of it, or it could be a first patch in the set, since it may
be used independently. What do you think?
Thanks for the patch!
Sawada-san was proposing another 2PC patch at [1]. Do you have any thoughts
about pros and cons between your patch and Sawada-san's?
[1]
https://www.postgresql.org/message-id/ca+fd4k4z6_b1etevqamwqhu4rx7xsrn5orl7ohj4b5b6sw-...@mail.gmail.com
Thank you for the link!
After a quick look on the Sawada-san's patch set I think that there are two
major differences:
Thanks for sharing your thought! As far as I read your patch quickly,
I basically agree with your this view.
1. There is a built-in foreign xacts resolver in the [1], which should be much
more convenient from the end-user perspective. It involves huge in-core changes
and additional complexity that is of course worth of.
However, it's still not clear for me that it is possible to resolve all foreign
prepared xacts on the Postgres' own side with a 100% guarantee. Imagine a
situation when the coordinator node is actually a HA cluster group (primary +
sync + async replica) and it failed just after PREPARE stage of after local
COMMIT. In that case all foreign xacts will be left in the prepared state.
After failover process complete synchronous replica will become a new primary.
Would it have all required info to properly resolve orphan prepared xacts?
IIUC, yes, the information required for automatic resolution is
WAL-logged and the standby tries to resolve those orphan transactions
from WAL after the failover. But Sawada-san's patch provides
the special function for manual resolution, so there may be some cases
where manual resolution is necessary.
Probably, this situation is handled properly in the [1], but I've not yet
finished a thorough reading of the patch set, though it has a great doc!
On the other hand, previous 0003 and my proposed patch rely on either manual
resolution of hung prepared xacts or usage of external monitor/resolver. This
approach is much simpler from the in-core perspective, but doesn't look as
complete as [1] though.
2. In the patch from this thread all 2PC logic sit in the postgres_fdw, while
[1] tries to put it into the generic fdw core, which also feels like a more
general and architecturally correct way. However, how many from the currently
available dozens of various FDWs are capable to perform 2PC? And how many of
them are maintained well enough to adopt this new API? This is not an argument
against [1] actually, since postgres_fdw is known to be the most advanced FDW
and an early adopter of new feature, just a little doubt about a usefulness of
this preliminary generalisation.
If we implement 2PC feature only for PostgreSQL sharding using
postgres_fdw, IMO it's ok to support only postgres_fdw.
But if we implement 2PC as the improvement on FDW independently
from PostgreSQL sharding and global visibility, I think that it's
necessary to support other FDW. I'm not sure how many FDW
actually will support this new 2PC interface. But if the interface is
not so complicated, I *guess* some FDW will support it in the near future.
Implementing 2PC feature only inside postgres_fdw seems to cause
another issue; COMMIT PREPARED is issued to the remote servers
after marking the local transaction as committed
(i.e., ProcArrayEndTransaction()). Is this safe? This issue happens
because COMMIT PREPARED is issued via
CallXactCallbacks(XACT_EVENT_COMMIT) and that CallXactCallbacks()
is called after ProcArrayEndTransaction().
Anyway, I think that [1] is a great work and really hope to find more time to
investigate it deeper later this year.
I'm sure your work is also great! I hope we can discuss the design
of 2PC feature together!
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
