Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
On Tue, 18 Feb 2020 at 00:40, Muhammad Usama wrote: > > Hi Sawada San, > > I have a couple of comments on > "v27-0002-Support-atomic-commit-among-multiple-foreign-ser.patch" > > 1- As part of the XLogReadRecord refactoring commit the signature of > XLogReadRecord was changed, > so the function call to XLogReadRecord() needs a small adjustment. > > i.e. In function XlogReadFdwXactData(XLogRecPtr lsn, char **buf, int *len) > ... > - record = XLogReadRecord(xlogreader, lsn, &errormsg); > + XLogBeginRead(xlogreader, lsn) > + record = XLogReadRecord(xlogreader, &errormsg); > > 2- In register_fdwxact(..) function you are setting the > XACT_FLAGS_FDWNOPREPARE transaction flag > when the register request comes in for foreign server that does not support > two-phase commit regardless > of the value of 'bool modified' argument. And later in the > PreCommit_FdwXacts() you just error out when > "foreign_twophase_commit" is set to 'required' only by looking at the > XACT_FLAGS_FDWNOPREPARE flag. > which I think is not correct. > Since there is a possibility that the transaction might have only read from > the foreign servers (not capable of > handling transactions or two-phase commit) and all other servers where we > require to do atomic commit > are capable enough of doing so. > If I am not missing something obvious here, then IMHO the > XACT_FLAGS_FDWNOPREPARE flag should only > be set when the transaction management/two-phase functionality is not > available and "modified" argument is > true in register_fdwxact() > Thank you for reviewing this patch! Your comments are incorporated in the latest patch set I recently sent[1]. [1] https://www.postgresql.org/message-id/CA%2Bfd4k5ZcDvoiY_5c-mF1oDACS5nUWS7ppoiOwjCOnM%2BgrJO-Q%40mail.gmail.com Regards, -- Masahiko Sawadahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
Hi Sawada San, I have a couple of comments on "v27-0002-Support-atomic-commit-among-multiple-foreign-ser.patch" 1- As part of the XLogReadRecord refactoring commit the signature of XLogReadRecord was changed, so the function call to XLogReadRecord() needs a small adjustment. i.e. In function XlogReadFdwXactData(XLogRecPtr lsn, char **buf, int *len) ... - record = XLogReadRecord(xlogreader, lsn, &errormsg); + XLogBeginRead(xlogreader, lsn) + record = XLogReadRecord(xlogreader, &errormsg); 2- In register_fdwxact(..) function you are setting the XACT_FLAGS_FDWNOPREPARE transaction flag when the register request comes in for foreign server that does not support two-phase commit regardless of the value of 'bool modified' argument. And later in the PreCommit_FdwXacts() you just error out when "foreign_twophase_commit" is set to 'required' only by looking at the XACT_FLAGS_FDWNOPREPARE flag. which I think is not correct. Since there is a possibility that the transaction might have only read from the foreign servers (not capable of handling transactions or two-phase commit) and all other servers where we require to do atomic commit are capable enough of doing so. If I am not missing something obvious here, then IMHO the XACT_FLAGS_FDWNOPREPARE flag should only be set when the transaction management/two-phase functionality is not available and "modified" argument is true in register_fdwxact() Thanks Best regards Muhammad Usama Highgo Software (Canada/China/Pakistan) The new status of this patch is: Waiting on Author
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
On Wed, Sep 04, 2019 at 12:44:20PM +0900, Masahiko Sawada wrote: > I forgot to include some new header files. Attached the updated patches. No reviews since and the patch does not apply anymore. I am moving it to next CF, waiting on author. -- Michael signature.asc Description: PGP signature
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
Hello Sawada-san, On 2019-Jul-02, Masahiko Sawada wrote: > On Mon, Jul 1, 2019 at 8:32 PM Thomas Munro wrote: > > Can we please have a fresh rebase? > > Thank you for the notice. Attached rebased patches. ... and again? -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
On Wed, Apr 17, 2019 at 10:23 PM Masahiko Sawada wrote: > Sorry for the very late. Attached updated version patches. Hello Sawada-san, Can we please have a fresh rebase? Thanks, -- Thomas Munro https://enterprisedb.com
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
On Thu, Jan 31, 2019 at 11:09:09AM +0100, Masahiko Sawada wrote: > Thanks. Actually I'm updating the patch set, changing API interface as > I proposed before and improving the document and README. I'll submit > the latest patch next week. Cool, I have moved the patch to next CF. -- Michael signature.asc Description: PGP signature
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
On Tue, Jan 29, 2019 at 5:47 PM Ildar Musin wrote: > > Hello, > > The patch needs rebase as it doesn't apply to the current master. I applied it > to the older commit to test it. It worked fine so far. Thank you for testing the patch! > > I found one bug though which would cause resolver to finish by timeout even > though there are unresolved foreign transactions in the list. The > `fdw_xact_exists()` function expects database id as the first argument and xid > as the second. But everywhere it is called arguments specified in the > different > order (xid first, then dbid). Also function declaration in header doesn't > match its definition. Will fix. > > There are some other things I found. > * In `FdwXactResolveAllDanglingTransactions()` variable `n_resolved` is > declared as bool but used as integer. > * In fdwxact.c's module comment there are > `FdwXactRegisterForeignTransaction()` > and `FdwXactMarkForeignTransactionModified()` functions mentioned that are > not there anymore. > * In documentation (storage.sgml) there is no mention of `pg_fdw_xact` > directory. > > Couple of stylistic notes. > * In `FdwXactCtlData struct` there are both camel case and snake case naming > used. > * In `get_fdw_xacts()` `xid != InvalidTransactionId` can be replaced with > `TransactionIdIsValid(xid)`. > * In `generate_fdw_xact_identifier()` the `fx` prefix could be a part of > format > string instead of being processed by `sprintf` as an extra argument. > I'll incorporate them at the next patch set. > I'll continue looking into the patch. Thanks! Thanks. Actually I'm updating the patch set, changing API interface as I proposed before and improving the document and README. I'll submit the latest patch next week. -- Regards, -- Masahiko Sawada NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
Hello, The patch needs rebase as it doesn't apply to the current master. I applied it to the older commit to test it. It worked fine so far. I found one bug though which would cause resolver to finish by timeout even though there are unresolved foreign transactions in the list. The `fdw_xact_exists()` function expects database id as the first argument and xid as the second. But everywhere it is called arguments specified in the different order (xid first, then dbid). Also function declaration in header doesn't match its definition. There are some other things I found. * In `FdwXactResolveAllDanglingTransactions()` variable `n_resolved` is declared as bool but used as integer. * In fdwxact.c's module comment there are `FdwXactRegisterForeignTransaction()` and `FdwXactMarkForeignTransactionModified()` functions mentioned that are not there anymore. * In documentation (storage.sgml) there is no mention of `pg_fdw_xact` directory. Couple of stylistic notes. * In `FdwXactCtlData struct` there are both camel case and snake case naming used. * In `get_fdw_xacts()` `xid != InvalidTransactionId` can be replaced with `TransactionIdIsValid(xid)`. * In `generate_fdw_xact_identifier()` the `fx` prefix could be a part of format string instead of being processed by `sprintf` as an extra argument. I'll continue looking into the patch. Thanks! On Tue, Nov 20, 2018 at 12:18 PM Masahiko Sawada wrote: > On Thu, Nov 15, 2018 at 7:36 PM Masahiko Sawada > wrote: > > > > On Mon, Oct 29, 2018 at 6:03 PM Masahiko Sawada > wrote: > > > > > > On Mon, Oct 29, 2018 at 10:16 AM Masahiko Sawada < > sawada.m...@gmail.com> wrote: > > > > > > > > On Wed, Oct 24, 2018 at 9:06 AM Masahiko Sawada < > sawada.m...@gmail.com> wrote: > > > > > > > > > > On Tue, Oct 23, 2018 at 12:54 PM Kyotaro HORIGUCHI > > > > > wrote: > > > > > > > > > > > > Hello. > > > > > > > > > > > > # It took a long time to come here.. > > > > > > > > > > > > At Fri, 19 Oct 2018 21:38:35 +0900, Masahiko Sawada < > sawada.m...@gmail.com> wrote in > > > > > > > > On Wed, Oct 10, 2018 at 1:34 PM Masahiko Sawada < > sawada.m...@gmail.com> wrote: > > > > > > ... > > > > > > > * Updated docs, added the new section "Distributed > Transaction" at > > > > > > > Chapter 33 to explain the concept to users > > > > > > > > > > > > > > * Moved atomic commit codes into src/backend/access/fdwxact > directory. > > > > > > > > > > > > > > * Some bug fixes. > > > > > > > > > > > > > > Please reivew them. > > > > > > > > > > > > I have some comments, with apologize in advance for possible > > > > > > duplicate or conflict with others' comments so far. > > > > > > > > > > Thank youf so much for reviewing this patch! > > > > > > > > > > > > > > > > > 0001: > > > > > > > > > > > > This sets XACT_FLAG_WROTENONTEMPREL when RELPERSISTENT_PERMANENT > > > > > > relation is modified. Isn't it needed when UNLOGGED tables are > > > > > > modified? It may be better that we have dedicated classification > > > > > > macro or function. > > > > > > > > > > I think even if we do atomic commit for modifying the an UNLOGGED > > > > > table and a remote table the data will get inconsistent if the > local > > > > > server crashes. For example, if the local server crashes after > > > > > prepared the transaction on foreign server but before the local > commit > > > > > and, we will lose the all data of the local UNLOGGED table whereas > the > > > > > modification of remote table is rollbacked. In case of persistent > > > > > tables, the data consistency is left. So I think the keeping data > > > > > consistency between remote data and local unlogged table is > difficult > > > > > and want to leave it as a restriction for now. Am I missing > something? > > > > > > > > > > > > > > > > > The flag is handled in heapam.c. I suppose that it should be done > > > > > > in the upper layer considering coming pluggable storage. > > > > > > (X_F_ACCESSEDTEMPREL is set in heapam, but..) > > > > > > > > > > > > > > > > Yeah, or we can set the flag after heap_insert in ExecInsert. > > > > > > > > > > > > > > > > > 0002: > > > > > > > > > > > > The name FdwXactParticipantsForAC doesn't sound good for me. How > > > > > > about FdwXactAtomicCommitPartitcipants? > > > > > > > > > > +1, will fix it. > > > > > > > > > > > > > > > > > Well, as the file comment of fdwxact.c, > > > > > > FdwXactRegisterTransaction is called from FDW driver and > > > > > > F_X_MarkForeignTransactionModified is called from executor. I > > > > > > think that we should clarify who is responsible to the whole > > > > > > sequence. Since the state of local tables affects, I suppose > > > > > > executor is that. Couldn't we do the whole thing within executor > > > > > > side? I'm not sure but I feel that > > > > > > F_X_RegisterForeignTransaction can be a part of > > > > > > F_X_MarkForeignTransactionModified. The callers of > > > > > > MarkForeignTransactionModified can find whether the table is > > > > > > involved in 2pc by IsT
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
On Thu, Nov 15, 2018 at 7:36 PM Masahiko Sawada wrote:
>
> On Mon, Oct 29, 2018 at 6:03 PM Masahiko Sawada wrote:
> >
> > On Mon, Oct 29, 2018 at 10:16 AM Masahiko Sawada
> > wrote:
> > >
> > > On Wed, Oct 24, 2018 at 9:06 AM Masahiko Sawada
> > > wrote:
> > > >
> > > > On Tue, Oct 23, 2018 at 12:54 PM Kyotaro HORIGUCHI
> > > > wrote:
> > > > >
> > > > > Hello.
> > > > >
> > > > > # It took a long time to come here..
> > > > >
> > > > > At Fri, 19 Oct 2018 21:38:35 +0900, Masahiko Sawada
> > > > > wrote in
> > > > >
> > > > > > On Wed, Oct 10, 2018 at 1:34 PM Masahiko Sawada
> > > > > > wrote:
> > > > > ...
> > > > > > * Updated docs, added the new section "Distributed Transaction" at
> > > > > > Chapter 33 to explain the concept to users
> > > > > >
> > > > > > * Moved atomic commit codes into src/backend/access/fdwxact
> > > > > > directory.
> > > > > >
> > > > > > * Some bug fixes.
> > > > > >
> > > > > > Please reivew them.
> > > > >
> > > > > I have some comments, with apologize in advance for possible
> > > > > duplicate or conflict with others' comments so far.
> > > >
> > > > Thank youf so much for reviewing this patch!
> > > >
> > > > >
> > > > > 0001:
> > > > >
> > > > > This sets XACT_FLAG_WROTENONTEMPREL when RELPERSISTENT_PERMANENT
> > > > > relation is modified. Isn't it needed when UNLOGGED tables are
> > > > > modified? It may be better that we have dedicated classification
> > > > > macro or function.
> > > >
> > > > I think even if we do atomic commit for modifying the an UNLOGGED
> > > > table and a remote table the data will get inconsistent if the local
> > > > server crashes. For example, if the local server crashes after
> > > > prepared the transaction on foreign server but before the local commit
> > > > and, we will lose the all data of the local UNLOGGED table whereas the
> > > > modification of remote table is rollbacked. In case of persistent
> > > > tables, the data consistency is left. So I think the keeping data
> > > > consistency between remote data and local unlogged table is difficult
> > > > and want to leave it as a restriction for now. Am I missing something?
> > > >
> > > > >
> > > > > The flag is handled in heapam.c. I suppose that it should be done
> > > > > in the upper layer considering coming pluggable storage.
> > > > > (X_F_ACCESSEDTEMPREL is set in heapam, but..)
> > > > >
> > > >
> > > > Yeah, or we can set the flag after heap_insert in ExecInsert.
> > > >
> > > > >
> > > > > 0002:
> > > > >
> > > > > The name FdwXactParticipantsForAC doesn't sound good for me. How
> > > > > about FdwXactAtomicCommitPartitcipants?
> > > >
> > > > +1, will fix it.
> > > >
> > > > >
> > > > > Well, as the file comment of fdwxact.c,
> > > > > FdwXactRegisterTransaction is called from FDW driver and
> > > > > F_X_MarkForeignTransactionModified is called from executor. I
> > > > > think that we should clarify who is responsible to the whole
> > > > > sequence. Since the state of local tables affects, I suppose
> > > > > executor is that. Couldn't we do the whole thing within executor
> > > > > side? I'm not sure but I feel that
> > > > > F_X_RegisterForeignTransaction can be a part of
> > > > > F_X_MarkForeignTransactionModified. The callers of
> > > > > MarkForeignTransactionModified can find whether the table is
> > > > > involved in 2pc by IsTwoPhaseCommitEnabled interface.
> > > >
> > > > Indeed. We can register foreign servers by executor while FDWs don't
> > > > need to register anything. I will remove the registration function so
> > > > that FDW developers don't need to call the register function but only
> > > > need to provide atomic commit APIs.
> > > >
> > > > >
> > > > >
> > > > > > if (foreign_twophase_commit == true &&
> > > > > > ((MyXactFlags & XACT_FLAGS_FDWNOPREPARE) != 0) )
> > > > > > ereport(ERROR,
> > > > > >
> > > > > > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> > > > > >errmsg("cannot COMMIT a distributed
> > > > > > transaction that has operated on foreign server that doesn't
> > > > > > support atomic commit")));
> > > > >
> > > > > The error is emitted when a the GUC is turned off in the
> > > > > trasaction where MarkTransactionModify'ed. I think that the
> > > > > number of the variables' possible states should be reduced for
> > > > > simplicity. For example in the case, once foreign_twopase_commit
> > > > > is checked in a transaction, subsequent changes in the
> > > > > transaction should be ignored during the transaction.
> > > > >
> > > >
> > > > I might have not gotten your comment correctly but since the
> > > > foreign_twophase_commit is a PGC_USERSET parameter I think we need to
> > > > check it at commit time. Also we need to keep participant servers even
> > > > when foreign_twophase_commit is off if both max_prepared_foreign_xacts
> > > > and max_foreign_xact_resolvers are > 0.
> > > >
> > > > I will post the updated patch in
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
On Tue, Oct 23, 2018 at 12:54 PM Kyotaro HORIGUCHI
wrote:
>
> Hello.
>
> # It took a long time to come here..
>
> At Fri, 19 Oct 2018 21:38:35 +0900, Masahiko Sawada
> wrote in
> > On Wed, Oct 10, 2018 at 1:34 PM Masahiko Sawada
> > wrote:
> ...
> > * Updated docs, added the new section "Distributed Transaction" at
> > Chapter 33 to explain the concept to users
> >
> > * Moved atomic commit codes into src/backend/access/fdwxact directory.
> >
> > * Some bug fixes.
> >
> > Please reivew them.
>
> I have some comments, with apologize in advance for possible
> duplicate or conflict with others' comments so far.
Thank youf so much for reviewing this patch!
>
> 0001:
>
> This sets XACT_FLAG_WROTENONTEMPREL when RELPERSISTENT_PERMANENT
> relation is modified. Isn't it needed when UNLOGGED tables are
> modified? It may be better that we have dedicated classification
> macro or function.
I think even if we do atomic commit for modifying the an UNLOGGED
table and a remote table the data will get inconsistent if the local
server crashes. For example, if the local server crashes after
prepared the transaction on foreign server but before the local commit
and, we will lose the all data of the local UNLOGGED table whereas the
modification of remote table is rollbacked. In case of persistent
tables, the data consistency is left. So I think the keeping data
consistency between remote data and local unlogged table is difficult
and want to leave it as a restriction for now. Am I missing something?
>
> The flag is handled in heapam.c. I suppose that it should be done
> in the upper layer considering coming pluggable storage.
> (X_F_ACCESSEDTEMPREL is set in heapam, but..)
>
Yeah, or we can set the flag after heap_insert in ExecInsert.
>
> 0002:
>
> The name FdwXactParticipantsForAC doesn't sound good for me. How
> about FdwXactAtomicCommitPartitcipants?
+1, will fix it.
>
> Well, as the file comment of fdwxact.c,
> FdwXactRegisterTransaction is called from FDW driver and
> F_X_MarkForeignTransactionModified is called from executor. I
> think that we should clarify who is responsible to the whole
> sequence. Since the state of local tables affects, I suppose
> executor is that. Couldn't we do the whole thing within executor
> side? I'm not sure but I feel that
> F_X_RegisterForeignTransaction can be a part of
> F_X_MarkForeignTransactionModified. The callers of
> MarkForeignTransactionModified can find whether the table is
> involved in 2pc by IsTwoPhaseCommitEnabled interface.
Indeed. We can register foreign servers by executor while FDWs don't
need to register anything. I will remove the registration function so
that FDW developers don't need to call the register function but only
need to provide atomic commit APIs.
>
>
> > if (foreign_twophase_commit == true &&
> > ((MyXactFlags & XACT_FLAGS_FDWNOPREPARE) != 0) )
> > ereport(ERROR,
> > (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> >errmsg("cannot COMMIT a distributed
> > transaction that has operated on foreign server that doesn't support atomic
> > commit")));
>
> The error is emitted when a the GUC is turned off in the
> trasaction where MarkTransactionModify'ed. I think that the
> number of the variables' possible states should be reduced for
> simplicity. For example in the case, once foreign_twopase_commit
> is checked in a transaction, subsequent changes in the
> transaction should be ignored during the transaction.
>
I might have not gotten your comment correctly but since the
foreign_twophase_commit is a PGC_USERSET parameter I think we need to
check it at commit time. Also we need to keep participant servers even
when foreign_twophase_commit is off if both max_prepared_foreign_xacts
and max_foreign_xact_resolvers are > 0.
I will post the updated patch in this week.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
Hello.
# It took a long time to come here..
At Fri, 19 Oct 2018 21:38:35 +0900, Masahiko Sawada
wrote in
> On Wed, Oct 10, 2018 at 1:34 PM Masahiko Sawada wrote:
...
> * Updated docs, added the new section "Distributed Transaction" at
> Chapter 33 to explain the concept to users
>
> * Moved atomic commit codes into src/backend/access/fdwxact directory.
>
> * Some bug fixes.
>
> Please reivew them.
I have some comments, with apologize in advance for possible
duplicate or conflict with others' comments so far.
0001:
This sets XACT_FLAG_WROTENONTEMPREL when RELPERSISTENT_PERMANENT
relation is modified. Isn't it needed when UNLOGGED tables are
modified? It may be better that we have dedicated classification
macro or function.
The flag is handled in heapam.c. I suppose that it should be done
in the upper layer considering coming pluggable storage.
(X_F_ACCESSEDTEMPREL is set in heapam, but..)
0002:
The name FdwXactParticipantsForAC doesn't sound good for me. How
about FdwXactAtomicCommitPartitcipants?
Well, as the file comment of fdwxact.c,
FdwXactRegisterTransaction is called from FDW driver and
F_X_MarkForeignTransactionModified is called from executor. I
think that we should clarify who is responsible to the whole
sequence. Since the state of local tables affects, I suppose
executor is that. Couldn't we do the whole thing within executor
side? I'm not sure but I feel that
F_X_RegisterForeignTransaction can be a part of
F_X_MarkForeignTransactionModified. The callers of
MarkForeignTransactionModified can find whether the table is
involved in 2pc by IsTwoPhaseCommitEnabled interface.
> if (foreign_twophase_commit == true &&
> ((MyXactFlags & XACT_FLAGS_FDWNOPREPARE) != 0) )
> ereport(ERROR,
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
>errmsg("cannot COMMIT a distributed
> transaction that has operated on foreign server that doesn't support atomic
> commit")));
The error is emitted when a the GUC is turned off in the
trasaction where MarkTransactionModify'ed. I think that the
number of the variables' possible states should be reduced for
simplicity. For example in the case, once foreign_twopase_commit
is checked in a transaction, subsequent changes in the
transaction should be ignored during the transaction.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
On Wed, Oct 3, 2018 at 6:02 PM Chris Travers wrote: > > > > On Wed, Oct 3, 2018 at 9:41 AM Chris Travers wrote: >> >> The following review has been posted through the commitfest application: >> make installcheck-world: tested, failed >> Implements feature: not tested >> Spec compliant: not tested >> Documentation:tested, failed >> >> I am hoping I am not out of order in writing this before the commitfest >> starts. The patch is big and long and so wanted to start on this while >> traffic is slow. >> >> I find this patch quite welcome and very close to a minimum viable version. >> The few significant limitations can be resolved later. One thing I may have >> missed in the documentation is a discussion of the limits of the current >> approach. I think this would be important to document because the caveats >> of the current approach are significant, but the people who need it will >> have the knowledge to work with issues if they come up. >> >> The major caveat I see in our past discussions and (if I read the patch >> correctly) is that the resolver goes through global transactions >> sequentially and does not move on to the next until the previous one is >> resolved. This means that if I have a global transaction on server A, with >> foreign servers B and C, and I have another one on server A with foreign >> servers C and D, if server B goes down at the wrong moment, the background >> worker does not look like it will detect the failure and move on to try to >> resolve the second, so server D will have a badly set vacuum horizon until >> this is resolved. Also if I read the patch correctly, it looks like one can >> invoke SQL commands to remove the bad transaction to allow processing to >> continue and manual resolution (this is good and necessary because in this >> area there is no ability to have perfect recoverability without occasional >> administrative action). I would really like to see more documentation of >> failure cases and appropriate administrative action at present. Otherwise >> this is I think a minimum viable addition and I think we want it. >> >> It is possible i missed that in the documentation. If so, my objection >> stands aside. If it is welcome I am happy to take a first crack at such >> docs. > Thank you for reviewing the patch! > > After further testing I am pretty sure I misread the patch. It looks like > one can have multiple resolvers which can, in fact, work through a queue > together solving this problem. So the objection above is not valid and I > withdraw that objection. I will re-review the docs in light of the > experience. Actually the patch doesn't solve this problem; the foreign transaction resolver processes distributed transactions sequentially. But since one resolver process is responsible for one database the backend connecting to another database can complete the distributed transaction. I understood the your concern and agreed to solve this problem. I'll address it in the next patch. > >> >> >> To my mind thats the only blocker in the code (but see below). I can say >> without a doubt that I would expect we would use this feature once available. >> >> -- >> >> Testing however failed. >> >> make installcheck-world fails with errors like the following: >> >> -- Modify foreign server and raise an error >> BEGIN; >> INSERT INTO ft7_twophase VALUES(8); >> + ERROR: prepread foreign transactions are disabled >> + HINT: Set max_prepared_foreign_transactions to a nonzero value. >> INSERT INTO ft8_twophase VALUES(NULL); -- violation >> ! ERROR: current transaction is aborted, commands ignored until end of >> transaction block >> ROLLBACK; >> SELECT * FROM ft7_twophase; >> ! ERROR: prepread foreign transactions are disabled >> ! HINT: Set max_prepared_foreign_transactions to a nonzero value. >> SELECT * FROM ft8_twophase; >> ! ERROR: prepread foreign transactions are disabled >> ! HINT: Set max_prepared_foreign_transactions to a nonzero value. >> -- Rollback foreign transaction that involves both 2PC-capable >> -- and 2PC-non-capable foreign servers. >> BEGIN; >> INSERT INTO ft8_twophase VALUES(7); >> + ERROR: prepread foreign transactions are disabled >> + HINT: Set max_prepared_foreign_transactions to a nonzero value. >> INSERT INTO ft9_not_twophase VALUES(7); >> + ERROR: current transaction is aborted, commands ignored until end of >> transaction block >> ROLLBACK; >> SELECT * FROM ft8_twophase; >> ! ERROR: prepread foreign transactions are disabled >> ! HINT: Set max_prepared_foreign_transactions to a nonzero value. >> >> make installcheck in the contrib directory shows the same, so that's the >> easiest way of reproducing, at least on a new installation. I think the >> test cases will have to handle that sort of setup. The 'make installcheck' is a regression test mode to do the tests to the existing installation. If the installation dis
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
On Wed, Oct 3, 2018 at 9:41 AM Chris Travers wrote: > The following review has been posted through the commitfest application: > make installcheck-world: tested, failed > Implements feature: not tested > Spec compliant: not tested > Documentation:tested, failed > > I am hoping I am not out of order in writing this before the commitfest > starts. The patch is big and long and so wanted to start on this while > traffic is slow. > > I find this patch quite welcome and very close to a minimum viable > version. The few significant limitations can be resolved later. One thing > I may have missed in the documentation is a discussion of the limits of the > current approach. I think this would be important to document because the > caveats of the current approach are significant, but the people who need it > will have the knowledge to work with issues if they come up. > > The major caveat I see in our past discussions and (if I read the patch > correctly) is that the resolver goes through global transactions > sequentially and does not move on to the next until the previous one is > resolved. This means that if I have a global transaction on server A, with > foreign servers B and C, and I have another one on server A with foreign > servers C and D, if server B goes down at the wrong moment, the background > worker does not look like it will detect the failure and move on to try to > resolve the second, so server D will have a badly set vacuum horizon until > this is resolved. Also if I read the patch correctly, it looks like one > can invoke SQL commands to remove the bad transaction to allow processing > to continue and manual resolution (this is good and necessary because in > this area there is no ability to have perfect recoverability without > occasional administrative action). I would really like to see more > documentation of failure cases and appropriate administrative action at > present. Otherwise this is I think a minimum viable addition and I think > we want it. > > It is possible i missed that in the documentation. If so, my objection > stands aside. If it is welcome I am happy to take a first crack at such > docs. > After further testing I am pretty sure I misread the patch. It looks like one can have multiple resolvers which can, in fact, work through a queue together solving this problem. So the objection above is not valid and I withdraw that objection. I will re-review the docs in light of the experience. > > To my mind thats the only blocker in the code (but see below). I can say > without a doubt that I would expect we would use this feature once > available. > > -- > > Testing however failed. > > make installcheck-world fails with errors like the following: > > -- Modify foreign server and raise an error > BEGIN; > INSERT INTO ft7_twophase VALUES(8); > + ERROR: prepread foreign transactions are disabled > + HINT: Set max_prepared_foreign_transactions to a nonzero value. > INSERT INTO ft8_twophase VALUES(NULL); -- violation > ! ERROR: current transaction is aborted, commands ignored until end of > transaction block > ROLLBACK; > SELECT * FROM ft7_twophase; > ! ERROR: prepread foreign transactions are disabled > ! HINT: Set max_prepared_foreign_transactions to a nonzero value. > SELECT * FROM ft8_twophase; > ! ERROR: prepread foreign transactions are disabled > ! HINT: Set max_prepared_foreign_transactions to a nonzero value. > -- Rollback foreign transaction that involves both 2PC-capable > -- and 2PC-non-capable foreign servers. > BEGIN; > INSERT INTO ft8_twophase VALUES(7); > + ERROR: prepread foreign transactions are disabled > + HINT: Set max_prepared_foreign_transactions to a nonzero value. > INSERT INTO ft9_not_twophase VALUES(7); > + ERROR: current transaction is aborted, commands ignored until end of > transaction block > ROLLBACK; > SELECT * FROM ft8_twophase; > ! ERROR: prepread foreign transactions are disabled > ! HINT: Set max_prepared_foreign_transactions to a nonzero value. > > make installcheck in the contrib directory shows the same, so that's the > easiest way of reproducing, at least on a new installation. I think the > test cases will have to handle that sort of setup. > > make check in the contrib directory passes. > > For reasons of test failures, I am setting this back to waiting on author. > > -- > I had a few other thoughts that I figure are worth sharing with the > community on this patch with the idea that once it is in place, this may > open up more options for collaboration in the area of federated and > distributed storage generally. I could imagine other foreign data wrappers > using this API, and folks might want to refactor out the atomic handling > part so that extensions that do not use the foreign data wrapper structure > could use it as well (while this looks like a classic SQL/MED issue, I am > not sure that only foreign data wrappers woul
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
On Wed, Oct 3, 2018 at 9:56 AM Chris Travers
wrote:
>
>
> On Wed, Oct 3, 2018 at 9:41 AM Chris Travers
> wrote:
>
>>
>> (errmsg("preparing foreign transactions
> (max_prepared_foreign_transactions > 0) requires maX_foreign_xact_resolvers
> > 0")));
>
Two more critical notes here which I think are small blockers.
The error message above references a config variable that does not exist.
The correct name of the config parameter is
max_foreign_transaction_resolvers
Setting that along with the following to 10 caused the tests to pass, but
again it fails on default configs:
max_prepared_foreign_transactions, max_prepared_transactions
>
>
>
> --
> Best Regards,
> Chris Travers
> Head of Database
>
> Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com
> Saarbrücker Straße 37a, 10405 Berlin
>
>
--
Best Regards,
Chris Travers
Head of Database
Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com
Saarbrücker Straße 37a, 10405 Berlin
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
On Wed, Oct 3, 2018 at 9:41 AM Chris Travers
wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world: tested, failed
> Implements feature: not tested
> Spec compliant: not tested
> Documentation:tested, failed
>
Also one really minor point: I think this is a typo (maX vs max)?
(errmsg("preparing foreign transactions (max_prepared_foreign_transactions
> 0) requires maX_foreign_xact_resolvers > 0")));
--
Best Regards,
Chris Travers
Head of Database
Tel: +49 162 9037 210 | Skype: einhverfr | www.adjust.com
Saarbrücker Straße 37a, 10405 Berlin
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
The following review has been posted through the commitfest application: make installcheck-world: tested, failed Implements feature: not tested Spec compliant: not tested Documentation:tested, failed I am hoping I am not out of order in writing this before the commitfest starts. The patch is big and long and so wanted to start on this while traffic is slow. I find this patch quite welcome and very close to a minimum viable version. The few significant limitations can be resolved later. One thing I may have missed in the documentation is a discussion of the limits of the current approach. I think this would be important to document because the caveats of the current approach are significant, but the people who need it will have the knowledge to work with issues if they come up. The major caveat I see in our past discussions and (if I read the patch correctly) is that the resolver goes through global transactions sequentially and does not move on to the next until the previous one is resolved. This means that if I have a global transaction on server A, with foreign servers B and C, and I have another one on server A with foreign servers C and D, if server B goes down at the wrong moment, the background worker does not look like it will detect the failure and move on to try to resolve the second, so server D will have a badly set vacuum horizon until this is resolved. Also if I read the patch correctly, it looks like one can invoke SQL commands to remove the bad transaction to allow processing to continue and manual resolution (this is good and necessary because in this area there is no ability to have perfect recoverability without occasional administrative action). I would really like to see more documentation of failure cases and appropriate administrative action at present. Otherwise this is I think a minimum viable addition and I think we want it. It is possible i missed that in the documentation. If so, my objection stands aside. If it is welcome I am happy to take a first crack at such docs. To my mind thats the only blocker in the code (but see below). I can say without a doubt that I would expect we would use this feature once available. -- Testing however failed. make installcheck-world fails with errors like the following: -- Modify foreign server and raise an error BEGIN; INSERT INTO ft7_twophase VALUES(8); + ERROR: prepread foreign transactions are disabled + HINT: Set max_prepared_foreign_transactions to a nonzero value. INSERT INTO ft8_twophase VALUES(NULL); -- violation ! ERROR: current transaction is aborted, commands ignored until end of transaction block ROLLBACK; SELECT * FROM ft7_twophase; ! ERROR: prepread foreign transactions are disabled ! HINT: Set max_prepared_foreign_transactions to a nonzero value. SELECT * FROM ft8_twophase; ! ERROR: prepread foreign transactions are disabled ! HINT: Set max_prepared_foreign_transactions to a nonzero value. -- Rollback foreign transaction that involves both 2PC-capable -- and 2PC-non-capable foreign servers. BEGIN; INSERT INTO ft8_twophase VALUES(7); + ERROR: prepread foreign transactions are disabled + HINT: Set max_prepared_foreign_transactions to a nonzero value. INSERT INTO ft9_not_twophase VALUES(7); + ERROR: current transaction is aborted, commands ignored until end of transaction block ROLLBACK; SELECT * FROM ft8_twophase; ! ERROR: prepread foreign transactions are disabled ! HINT: Set max_prepared_foreign_transactions to a nonzero value. make installcheck in the contrib directory shows the same, so that's the easiest way of reproducing, at least on a new installation. I think the test cases will have to handle that sort of setup. make check in the contrib directory passes. For reasons of test failures, I am setting this back to waiting on author. -- I had a few other thoughts that I figure are worth sharing with the community on this patch with the idea that once it is in place, this may open up more options for collaboration in the area of federated and distributed storage generally. I could imagine other foreign data wrappers using this API, and folks might want to refactor out the atomic handling part so that extensions that do not use the foreign data wrapper structure could use it as well (while this looks like a classic SQL/MED issue, I am not sure that only foreign data wrappers would be interested in the API. The new status of this patch is: Waiting on Author
Re: [HACKERS] Transactions involving multiple postgres foreign servers, take 2
On Fri, Aug 03, 2018 at 05:52:24PM +0900, Masahiko Sawada wrote: > I attached the updated version patch as the previous versions conflict > with the current HEAD. Please note that the latest patch set does not apply anymore, so this patch is moved to next CF, waiting on author. -- Michael signature.asc Description: PGP signature
