Transactions involving multiple postgres foreign servers, take 2

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
168 messages Options
123456789
Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

Ahsan Hadi-2


On Fri, Jul 17, 2020 at 9:56 PM Fujii Masao <[hidden email]> wrote:


On 2020/07/16 14:47, Masahiko Sawada wrote:
> On Tue, 14 Jul 2020 at 11:19, Fujii Masao <[hidden email]> wrote:
>>
>>
>>
>> On 2020/07/14 9:08, Masahiro Ikeda wrote:
>>>> I've attached the latest version patches. I've incorporated the review
>>>> comments I got so far and improved locking strategy.
>>>
>>> Thanks for updating the patch!
>>
>> +1
>> I'm interested in these patches and now studying them. While checking
>> the behaviors of the patched PostgreSQL, I got three comments.
>
> Thank you for testing this patch!
>
>>
>> 1. We can access to the foreign table even during recovery in the HEAD.
>> But in the patched version, when I did that, I got the following error.
>> Is this intentional?
>>
>> ERROR:  cannot assign TransactionIds during recovery
>
> No, it should be fixed. I'm going to fix this by not collecting
> participants for atomic commit during recovery.

Thanks for trying to fix the issues!

I'd like to report one more issue. When I started new transaction
in the local server, executed INSERT in the remote server via
postgres_fdw and then quit psql, I got the following assertion failure.

TRAP: FailedAssertion("fdwxact", File: "fdwxact.c", Line: 1570)
0   postgres                            0x000000010d52f3c0 ExceptionalCondition + 160
1   postgres                            0x000000010cefbc49 ForgetAllFdwXactParticipants + 313
2   postgres                            0x000000010cefff14 AtProcExit_FdwXact + 20
3   postgres                            0x000000010d313fe3 shmem_exit + 179
4   postgres                            0x000000010d313e7a proc_exit_prepare + 122
5   postgres                            0x000000010d313da3 proc_exit + 19
6   postgres                            0x000000010d35112f PostgresMain + 3711
7   postgres                            0x000000010d27bb3a BackendRun + 570
8   postgres                            0x000000010d27af6b BackendStartup + 475
9   postgres                            0x000000010d279ed1 ServerLoop + 593
10  postgres                            0x000000010d277940 PostmasterMain + 6016
11  postgres                            0x000000010d1597b9 main + 761
12  libdyld.dylib                       0x00007fff7161e3d5 start + 1
13  ???                                 0x0000000000000003 0x0 + 3

I have done a test with the latest set of patches shared by Swada and I am not able to reproduce this issue. Started a prepared transaction on the local server and then did a couple of inserts in a remote table using postgres_fdw and the quit psql. I am not able to reproduce the assertion failure.

 

Regards,


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




--
Highgo Software (Canada/China/Pakistan)
URL : http://www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
EMAIL: mailto: [hidden email]
Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

Muhammad Usama
In reply to this post by Masahiko Sawada-2


On Wed, Jul 22, 2020 at 12:42 PM Masahiko Sawada <[hidden email]> wrote:
On Sat, 18 Jul 2020 at 01:55, Fujii Masao <[hidden email]> wrote:
>
>
>
> On 2020/07/16 14:47, Masahiko Sawada wrote:
> > On Tue, 14 Jul 2020 at 11:19, Fujii Masao <[hidden email]> wrote:
> >>
> >>
> >>
> >> On 2020/07/14 9:08, Masahiro Ikeda wrote:
> >>>> I've attached the latest version patches. I've incorporated the review
> >>>> comments I got so far and improved locking strategy.
> >>>
> >>> Thanks for updating the patch!
> >>
> >> +1
> >> I'm interested in these patches and now studying them. While checking
> >> the behaviors of the patched PostgreSQL, I got three comments.
> >
> > Thank you for testing this patch!
> >
> >>
> >> 1. We can access to the foreign table even during recovery in the HEAD.
> >> But in the patched version, when I did that, I got the following error.
> >> Is this intentional?
> >>
> >> ERROR:  cannot assign TransactionIds during recovery
> >
> > No, it should be fixed. I'm going to fix this by not collecting
> > participants for atomic commit during recovery.
>
> Thanks for trying to fix the issues!
>
> I'd like to report one more issue. When I started new transaction
> in the local server, executed INSERT in the remote server via
> postgres_fdw and then quit psql, I got the following assertion failure.
>
> TRAP: FailedAssertion("fdwxact", File: "fdwxact.c", Line: 1570)
> 0   postgres                            0x000000010d52f3c0 ExceptionalCondition + 160
> 1   postgres                            0x000000010cefbc49 ForgetAllFdwXactParticipants + 313
> 2   postgres                            0x000000010cefff14 AtProcExit_FdwXact + 20
> 3   postgres                            0x000000010d313fe3 shmem_exit + 179
> 4   postgres                            0x000000010d313e7a proc_exit_prepare + 122
> 5   postgres                            0x000000010d313da3 proc_exit + 19
> 6   postgres                            0x000000010d35112f PostgresMain + 3711
> 7   postgres                            0x000000010d27bb3a BackendRun + 570
> 8   postgres                            0x000000010d27af6b BackendStartup + 475
> 9   postgres                            0x000000010d279ed1 ServerLoop + 593
> 10  postgres                            0x000000010d277940 PostmasterMain + 6016
> 11  postgres                            0x000000010d1597b9 main + 761
> 12  libdyld.dylib                       0x00007fff7161e3d5 start + 1
> 13  ???                                 0x0000000000000003 0x0 + 3
>

Thank you for reporting the issue!

I've attached the latest version patch that incorporated all comments
I got so far. I've removed the patch adding the 'prefer' mode of
foreign_twophase_commit to keep the patch set simple.

I have started to review the patchset. Just a quick comment.

Patch v24-0002-Support-atomic-commit-among-multiple-foreign-ser.patch 
contains changes (adding fdwxact includes) for
src/backend/executor/nodeForeignscan.c,  src/backend/executor/nodeModifyTable.c
and  src/backend/executor/execPartition.c files that doesn't seem to be
required with the latest version.


Thanks
Best regards
Muhammad Usama

 

Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

Masahiko Sawada-2
On Thu, 23 Jul 2020 at 22:51, Muhammad Usama <[hidden email]> wrote:

>
>
>
> On Wed, Jul 22, 2020 at 12:42 PM Masahiko Sawada <[hidden email]> wrote:
>>
>> On Sat, 18 Jul 2020 at 01:55, Fujii Masao <[hidden email]> wrote:
>> >
>> >
>> >
>> > On 2020/07/16 14:47, Masahiko Sawada wrote:
>> > > On Tue, 14 Jul 2020 at 11:19, Fujii Masao <[hidden email]> wrote:
>> > >>
>> > >>
>> > >>
>> > >> On 2020/07/14 9:08, Masahiro Ikeda wrote:
>> > >>>> I've attached the latest version patches. I've incorporated the review
>> > >>>> comments I got so far and improved locking strategy.
>> > >>>
>> > >>> Thanks for updating the patch!
>> > >>
>> > >> +1
>> > >> I'm interested in these patches and now studying them. While checking
>> > >> the behaviors of the patched PostgreSQL, I got three comments.
>> > >
>> > > Thank you for testing this patch!
>> > >
>> > >>
>> > >> 1. We can access to the foreign table even during recovery in the HEAD.
>> > >> But in the patched version, when I did that, I got the following error.
>> > >> Is this intentional?
>> > >>
>> > >> ERROR:  cannot assign TransactionIds during recovery
>> > >
>> > > No, it should be fixed. I'm going to fix this by not collecting
>> > > participants for atomic commit during recovery.
>> >
>> > Thanks for trying to fix the issues!
>> >
>> > I'd like to report one more issue. When I started new transaction
>> > in the local server, executed INSERT in the remote server via
>> > postgres_fdw and then quit psql, I got the following assertion failure.
>> >
>> > TRAP: FailedAssertion("fdwxact", File: "fdwxact.c", Line: 1570)
>> > 0   postgres                            0x000000010d52f3c0 ExceptionalCondition + 160
>> > 1   postgres                            0x000000010cefbc49 ForgetAllFdwXactParticipants + 313
>> > 2   postgres                            0x000000010cefff14 AtProcExit_FdwXact + 20
>> > 3   postgres                            0x000000010d313fe3 shmem_exit + 179
>> > 4   postgres                            0x000000010d313e7a proc_exit_prepare + 122
>> > 5   postgres                            0x000000010d313da3 proc_exit + 19
>> > 6   postgres                            0x000000010d35112f PostgresMain + 3711
>> > 7   postgres                            0x000000010d27bb3a BackendRun + 570
>> > 8   postgres                            0x000000010d27af6b BackendStartup + 475
>> > 9   postgres                            0x000000010d279ed1 ServerLoop + 593
>> > 10  postgres                            0x000000010d277940 PostmasterMain + 6016
>> > 11  postgres                            0x000000010d1597b9 main + 761
>> > 12  libdyld.dylib                       0x00007fff7161e3d5 start + 1
>> > 13  ???                                 0x0000000000000003 0x0 + 3
>> >
>>
>> Thank you for reporting the issue!
>>
>> I've attached the latest version patch that incorporated all comments
>> I got so far. I've removed the patch adding the 'prefer' mode of
>> foreign_twophase_commit to keep the patch set simple.
>
>
> I have started to review the patchset. Just a quick comment.
>
> Patch v24-0002-Support-atomic-commit-among-multiple-foreign-ser.patch
> contains changes (adding fdwxact includes) for
> src/backend/executor/nodeForeignscan.c,  src/backend/executor/nodeModifyTable.c
> and  src/backend/executor/execPartition.c files that doesn't seem to be
> required with the latest version.

Thanks for your comment.

Right. I've removed these changes on the local branch.

Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

Fujii Masao-4


On 2020/07/27 15:59, Masahiko Sawada wrote:

> On Thu, 23 Jul 2020 at 22:51, Muhammad Usama <[hidden email]> wrote:
>>
>>
>>
>> On Wed, Jul 22, 2020 at 12:42 PM Masahiko Sawada <[hidden email]> wrote:
>>>
>>> On Sat, 18 Jul 2020 at 01:55, Fujii Masao <[hidden email]> wrote:
>>>>
>>>>
>>>>
>>>> On 2020/07/16 14:47, Masahiko Sawada wrote:
>>>>> On Tue, 14 Jul 2020 at 11:19, Fujii Masao <[hidden email]> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2020/07/14 9:08, Masahiro Ikeda wrote:
>>>>>>>> I've attached the latest version patches. I've incorporated the review
>>>>>>>> comments I got so far and improved locking strategy.
>>>>>>>
>>>>>>> Thanks for updating the patch!
>>>>>>
>>>>>> +1
>>>>>> I'm interested in these patches and now studying them. While checking
>>>>>> the behaviors of the patched PostgreSQL, I got three comments.
>>>>>
>>>>> Thank you for testing this patch!
>>>>>
>>>>>>
>>>>>> 1. We can access to the foreign table even during recovery in the HEAD.
>>>>>> But in the patched version, when I did that, I got the following error.
>>>>>> Is this intentional?
>>>>>>
>>>>>> ERROR:  cannot assign TransactionIds during recovery
>>>>>
>>>>> No, it should be fixed. I'm going to fix this by not collecting
>>>>> participants for atomic commit during recovery.
>>>>
>>>> Thanks for trying to fix the issues!
>>>>
>>>> I'd like to report one more issue. When I started new transaction
>>>> in the local server, executed INSERT in the remote server via
>>>> postgres_fdw and then quit psql, I got the following assertion failure.
>>>>
>>>> TRAP: FailedAssertion("fdwxact", File: "fdwxact.c", Line: 1570)
>>>> 0   postgres                            0x000000010d52f3c0 ExceptionalCondition + 160
>>>> 1   postgres                            0x000000010cefbc49 ForgetAllFdwXactParticipants + 313
>>>> 2   postgres                            0x000000010cefff14 AtProcExit_FdwXact + 20
>>>> 3   postgres                            0x000000010d313fe3 shmem_exit + 179
>>>> 4   postgres                            0x000000010d313e7a proc_exit_prepare + 122
>>>> 5   postgres                            0x000000010d313da3 proc_exit + 19
>>>> 6   postgres                            0x000000010d35112f PostgresMain + 3711
>>>> 7   postgres                            0x000000010d27bb3a BackendRun + 570
>>>> 8   postgres                            0x000000010d27af6b BackendStartup + 475
>>>> 9   postgres                            0x000000010d279ed1 ServerLoop + 593
>>>> 10  postgres                            0x000000010d277940 PostmasterMain + 6016
>>>> 11  postgres                            0x000000010d1597b9 main + 761
>>>> 12  libdyld.dylib                       0x00007fff7161e3d5 start + 1
>>>> 13  ???                                 0x0000000000000003 0x0 + 3
>>>>
>>>
>>> Thank you for reporting the issue!
>>>
>>> I've attached the latest version patch that incorporated all comments
>>> I got so far. I've removed the patch adding the 'prefer' mode of
>>> foreign_twophase_commit to keep the patch set simple.
>>
>>
>> I have started to review the patchset. Just a quick comment.
>>
>> Patch v24-0002-Support-atomic-commit-among-multiple-foreign-ser.patch
>> contains changes (adding fdwxact includes) for
>> src/backend/executor/nodeForeignscan.c,  src/backend/executor/nodeModifyTable.c
>> and  src/backend/executor/execPartition.c files that doesn't seem to be
>> required with the latest version.
>
> Thanks for your comment.
>
> Right. I've removed these changes on the local branch.

The latest patches failed to be applied to the master branch. Could you rebase the patches?

Regards,

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


Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

Masahiko Sawada-2
On Fri, 21 Aug 2020 at 00:36, Fujii Masao <[hidden email]> wrote:

>
>
>
> On 2020/07/27 15:59, Masahiko Sawada wrote:
> > On Thu, 23 Jul 2020 at 22:51, Muhammad Usama <[hidden email]> wrote:
> >>
> >>
> >>
> >> On Wed, Jul 22, 2020 at 12:42 PM Masahiko Sawada <[hidden email]> wrote:
> >>>
> >>> On Sat, 18 Jul 2020 at 01:55, Fujii Masao <[hidden email]> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On 2020/07/16 14:47, Masahiko Sawada wrote:
> >>>>> On Tue, 14 Jul 2020 at 11:19, Fujii Masao <[hidden email]> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 2020/07/14 9:08, Masahiro Ikeda wrote:
> >>>>>>>> I've attached the latest version patches. I've incorporated the review
> >>>>>>>> comments I got so far and improved locking strategy.
> >>>>>>>
> >>>>>>> Thanks for updating the patch!
> >>>>>>
> >>>>>> +1
> >>>>>> I'm interested in these patches and now studying them. While checking
> >>>>>> the behaviors of the patched PostgreSQL, I got three comments.
> >>>>>
> >>>>> Thank you for testing this patch!
> >>>>>
> >>>>>>
> >>>>>> 1. We can access to the foreign table even during recovery in the HEAD.
> >>>>>> But in the patched version, when I did that, I got the following error.
> >>>>>> Is this intentional?
> >>>>>>
> >>>>>> ERROR:  cannot assign TransactionIds during recovery
> >>>>>
> >>>>> No, it should be fixed. I'm going to fix this by not collecting
> >>>>> participants for atomic commit during recovery.
> >>>>
> >>>> Thanks for trying to fix the issues!
> >>>>
> >>>> I'd like to report one more issue. When I started new transaction
> >>>> in the local server, executed INSERT in the remote server via
> >>>> postgres_fdw and then quit psql, I got the following assertion failure.
> >>>>
> >>>> TRAP: FailedAssertion("fdwxact", File: "fdwxact.c", Line: 1570)
> >>>> 0   postgres                            0x000000010d52f3c0 ExceptionalCondition + 160
> >>>> 1   postgres                            0x000000010cefbc49 ForgetAllFdwXactParticipants + 313
> >>>> 2   postgres                            0x000000010cefff14 AtProcExit_FdwXact + 20
> >>>> 3   postgres                            0x000000010d313fe3 shmem_exit + 179
> >>>> 4   postgres                            0x000000010d313e7a proc_exit_prepare + 122
> >>>> 5   postgres                            0x000000010d313da3 proc_exit + 19
> >>>> 6   postgres                            0x000000010d35112f PostgresMain + 3711
> >>>> 7   postgres                            0x000000010d27bb3a BackendRun + 570
> >>>> 8   postgres                            0x000000010d27af6b BackendStartup + 475
> >>>> 9   postgres                            0x000000010d279ed1 ServerLoop + 593
> >>>> 10  postgres                            0x000000010d277940 PostmasterMain + 6016
> >>>> 11  postgres                            0x000000010d1597b9 main + 761
> >>>> 12  libdyld.dylib                       0x00007fff7161e3d5 start + 1
> >>>> 13  ???                                 0x0000000000000003 0x0 + 3
> >>>>
> >>>
> >>> Thank you for reporting the issue!
> >>>
> >>> I've attached the latest version patch that incorporated all comments
> >>> I got so far. I've removed the patch adding the 'prefer' mode of
> >>> foreign_twophase_commit to keep the patch set simple.
> >>
> >>
> >> I have started to review the patchset. Just a quick comment.
> >>
> >> Patch v24-0002-Support-atomic-commit-among-multiple-foreign-ser.patch
> >> contains changes (adding fdwxact includes) for
> >> src/backend/executor/nodeForeignscan.c,  src/backend/executor/nodeModifyTable.c
> >> and  src/backend/executor/execPartition.c files that doesn't seem to be
> >> required with the latest version.
> >
> > Thanks for your comment.
> >
> > Right. I've removed these changes on the local branch.
>
> The latest patches failed to be applied to the master branch. Could you rebase the patches?
>
Thank you for letting me know. I've attached the latest version patch set.

Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

v25-0005-Add-regression-tests-for-foreign-twophase-commit.patch (62K) Download Attachment
v25-0004-postgres_fdw-supports-atomic-commit-APIs.patch (62K) Download Attachment
v25-0001-Recreate-RemoveForeignServerById.patch (3K) Download Attachment
v25-0003-Documentation-update.patch (55K) Download Attachment
v25-0002-Support-atomic-commit-among-multiple-foreign-ser.patch (256K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

Masahiro Ikeda
In reply to this post by Masahiko Sawada-2
> On 2020-07-17 15:55, Masahiko Sawada wrote:
>> On Fri, 17 Jul 2020 at 11:06, Masahiro Ikeda
>> <ikedamsh(at)oss(dot)nttdata(dot)com>
>> wrote:
>>>
>>> On 2020-07-16 13:16, Masahiko Sawada wrote:
>>>> On Tue, 14 Jul 2020 at 17:24, Masahiro Ikeda
>>>> <ikedamsh(at)oss(dot)nttdata(dot)com>
>>>> wrote:
>>>>>
>>>>>> I've attached the latest version patches. I've incorporated the
>>>>>> review
>>>>>> comments I got so far and improved locking strategy.
>>>>>
>>>>> I want to ask a question about streaming replication with 2PC.
>>>>> Are you going to support 2PC with streaming replication?
>>>>>
>>>>> I tried streaming replication using v23 patches.
>>>>> I confirm that 2PC works with streaming replication,
>>>>> which there are primary/standby coordinator.
>>>>>
>>>>> But, in my understanding, the WAL of "PREPARE" and
>>>>> "COMMIT/ABORT PREPARED" can't be replicated to the standby server
>>>>> in
>>>>> sync.
>>>>>
>>>>> If this is right, the unresolved transaction can be occurred.
>>>>>
>>>>> For example,
>>>>>
>>>>> 1. PREPARE is done
>>>>> 2. crash primary before the WAL related to PREPARE is
>>>>>     replicated to the standby server
>>>>> 3. promote standby server // but can't execute "ABORT PREPARED"
>>>>>
>>>>> In above case, the remote server has the unresolved transaction.
>>>>> Can we solve this problem to support in-sync replication?
>>>>>
>>>>> But, I think some users use async replication for performance.
>>>>> Do we need to document the limitation or make another solution?
>>>>>
>>>>
>>>> IIUC with synchronous replication, we can guarantee that WAL records
>>>> are written on both primary and replicas when the client got an
>>>> acknowledgment of commit. We don't replicate each WAL records
>>>> generated during transaction one by one in sync. In the case you
>>>> described, the client will get an error due to the server crash.
>>>> Therefore I think the user cannot expect WAL records generated so
>>>> far
>>>> has been replicated. The same issue could happen also when the user
>>>> executes PREPARE TRANSACTION and the server crashes.
>>>
>>> Thanks! I didn't noticed the behavior when a user executes PREPARE
>>> TRANSACTION is same.
>>>
>>> IIUC with 2PC, there is a different point between (1)PREPARE
>>> TRANSACTION
>>> and (2)2PC.
>>> The point is that whether the client can know when the server crashed
>>> and it's global tx id.
>>>
>>> If (1)PREPARE TRANSACTION is failed, it's ok the client execute same
>>> command
>>> because if the remote server is already prepared the command will be
>>> ignored.
>>>
>>> But, if (2)2PC is failed with coordinator crash, the client can't
>>> know
>>> what operations should be done.
>>>
>>> If the old coordinator already executed PREPARED, there are some
>>> transaction which should be ABORT PREPARED.
>>> But if the PREPARED WAL is not sent to the standby, the new
>>> coordinator
>>> can't execute ABORT PREPARED.
>>> And the client can't know which remote servers have PREPARED
>>> transactions which should be ABORTED either.
>>>
>>> Even if the client can know that, only the old coordinator knows its
>>> global transaction id.
>>> Only the database administrator can analyze the old coordinator's log
>>> and then execute the appropriate commands manually, right?
>>
>> I think that's right. In the case of the coordinator crash, the user
>> can look orphaned foreign prepared transactions by checking the
>> 'identifier' column of pg_foreign_xacts on the new standby server and
>> the prepared transactions on the remote servers.
>>
> I think there is a case we can't check orphaned foreign
> prepared transaction in pg_foreign_xacts view on the new standby
> server.
> It confuses users and database administrators.
>
> If the primary coordinator crashes after preparing foreign transaction,
> but before sending XLOG_FDWXACT_INSERT records to the standby server,
> the standby server can't restore their transaction status and
> pg_foreign_xacts view doesn't show the prepared foreign transactions.
>
> To send XLOG_FDWXACT_INSERT records asynchronously leads this problem.

If the primary replicates XLOG_FDWXACT_INSERT to the standby
asynchronously,
some prepared transaction may be unsolved forever.

Since I think to solve this inconsistency manually is hard operation,
we need to support synchronous XLOG_FDWXACT_INSERT replication.

I understood that there are a lot of impact to the performance,
but users can control the consistency/durability vs performance
with synchronous_commit parameter.

What do you think?


> Thank you for letting me know. I've attached the latest version patch
> set.

Thanks for updating.
But, the latest patches failed to be applied to the master branch.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION


Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

Masahiko Sawada-2
On Fri, 28 Aug 2020 at 17:50, Masahiro Ikeda <[hidden email]> wrote:

>
> > I think there is a case we can't check orphaned foreign
> > prepared transaction in pg_foreign_xacts view on the new standby
> > server.
> > It confuses users and database administrators.
> >
> > If the primary coordinator crashes after preparing foreign transaction,
> > but before sending XLOG_FDWXACT_INSERT records to the standby server,
> > the standby server can't restore their transaction status and
> > pg_foreign_xacts view doesn't show the prepared foreign transactions.
> >
> > To send XLOG_FDWXACT_INSERT records asynchronously leads this problem.
>
> If the primary replicates XLOG_FDWXACT_INSERT to the standby
> asynchronously,
> some prepared transaction may be unsolved forever.
>
> Since I think to solve this inconsistency manually is hard operation,
> we need to support synchronous XLOG_FDWXACT_INSERT replication.
>
> I understood that there are a lot of impact to the performance,
> but users can control the consistency/durability vs performance
> with synchronous_commit parameter.
>
> What do you think?

I think the user can check such prepared transactions by seeing
transactions that exist on the foreign server's pg_prepared_xact but
not on the coordinator server's pg_foreign_xacts, no? To make checking
such prepared transactions easy, perhaps we could contain the
timestamp to prepared transaction id. But I’m concerned the
duplication of transaction id due to clock skew.

If there is a way to identify such unresolved foreign transactions and
it's not cumbersome, given that the likelihood of problem you're
concerned is unlikely high I guess a certain number of would be able
to accept it as a restriction. So I’d recommend not dealing with this
problem in the first version patch and we will be able to improve this
feature to deal with this problem as an additional feature. Thoughts?

> > Thank you for letting me know. I've attached the latest version patch
> > set.
>
> Thanks for updating.
> But, the latest patches failed to be applied to the master branch.

I'll submit the updated version patch.

Regards,
--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

Masahiro Ikeda
On 2020-09-03 23:08, Masahiko Sawada wrote:

> On Fri, 28 Aug 2020 at 17:50, Masahiro Ikeda <[hidden email]>
> wrote:
>>
>> > I think there is a case we can't check orphaned foreign
>> > prepared transaction in pg_foreign_xacts view on the new standby
>> > server.
>> > It confuses users and database administrators.
>> >
>> > If the primary coordinator crashes after preparing foreign transaction,
>> > but before sending XLOG_FDWXACT_INSERT records to the standby server,
>> > the standby server can't restore their transaction status and
>> > pg_foreign_xacts view doesn't show the prepared foreign transactions.
>> >
>> > To send XLOG_FDWXACT_INSERT records asynchronously leads this problem.
>>
>> If the primary replicates XLOG_FDWXACT_INSERT to the standby
>> asynchronously,
>> some prepared transaction may be unsolved forever.
>>
>> Since I think to solve this inconsistency manually is hard operation,
>> we need to support synchronous XLOG_FDWXACT_INSERT replication.
>>
>> I understood that there are a lot of impact to the performance,
>> but users can control the consistency/durability vs performance
>> with synchronous_commit parameter.
>>
>> What do you think?
>
> I think the user can check such prepared transactions by seeing
> transactions that exist on the foreign server's pg_prepared_xact but
> not on the coordinator server's pg_foreign_xacts, no? To make checking
> such prepared transactions easy, perhaps we could contain the
> timestamp to prepared transaction id. But I’m concerned the
> duplication of transaction id due to clock skew.

Thanks for letting me know.
I agreed that we can check pg_prepared_xact and pg_foreign_xacts.

We have to abort the transaction which exists in pg_prepared_xact and
doesn't exist in pg_foreign_xacts manually, don't we?
So users have to use the foreign database which supports to show
prepared transaction status like pg_foreign_xacts.

When duplication of transaction id is made?
I'm sorry that I couldn't understand about clock skew.

IICU, since prepared id may have coordinator's xid, there is no clock
skew
and we can determine transaction_id uniquely.
If the fdw implements GetPrepareId_function API and it generates
transaction_id without coordinator's xid, your concern will emerge.
But, I can't understand the case to generate transaction_id without
coordinator's xid.

> If there is a way to identify such unresolved foreign transactions and
> it's not cumbersome, given that the likelihood of problem you're
> concerned is unlikely high I guess a certain number of would be able
> to accept it as a restriction. So I’d recommend not dealing with this
> problem in the first version patch and we will be able to improve this
> feature to deal with this problem as an additional feature. Thoughts?

I agree. Thanks for your comments.

>> > Thank you for letting me know. I've attached the latest version patch
>> > set.
>>
>> Thanks for updating.
>> But, the latest patches failed to be applied to the master branch.
>
> I'll submit the updated version patch.

Thanks.

Regards,
--
Masahiro Ikeda
NTT DATA CORPORATION


Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

Michael Paquier-2
In reply to this post by Masahiko Sawada-2
On Fri, Aug 21, 2020 at 03:25:29PM +0900, Masahiko Sawada wrote:
> Thank you for letting me know. I've attached the latest version patch set.

This needs a rebase.  Patch 0002 is conflicting with some of the
recent changes done in syncrep.c and procarray.c, at least.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

Fujii Masao-4
In reply to this post by Masahiko Sawada-2


On 2020/08/21 15:25, Masahiko Sawada wrote:

> On Fri, 21 Aug 2020 at 00:36, Fujii Masao <[hidden email]> wrote:
>>
>>
>>
>> On 2020/07/27 15:59, Masahiko Sawada wrote:
>>> On Thu, 23 Jul 2020 at 22:51, Muhammad Usama <[hidden email]> wrote:
>>>>
>>>>
>>>>
>>>> On Wed, Jul 22, 2020 at 12:42 PM Masahiko Sawada <[hidden email]> wrote:
>>>>>
>>>>> On Sat, 18 Jul 2020 at 01:55, Fujii Masao <[hidden email]> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2020/07/16 14:47, Masahiko Sawada wrote:
>>>>>>> On Tue, 14 Jul 2020 at 11:19, Fujii Masao <[hidden email]> wrote:
>>>>>>>>
>>>>>>>>
>>>>>>>>
>>>>>>>> On 2020/07/14 9:08, Masahiro Ikeda wrote:
>>>>>>>>>> I've attached the latest version patches. I've incorporated the review
>>>>>>>>>> comments I got so far and improved locking strategy.
>>>>>>>>>
>>>>>>>>> Thanks for updating the patch!
>>>>>>>>
>>>>>>>> +1
>>>>>>>> I'm interested in these patches and now studying them. While checking
>>>>>>>> the behaviors of the patched PostgreSQL, I got three comments.
>>>>>>>
>>>>>>> Thank you for testing this patch!
>>>>>>>
>>>>>>>>
>>>>>>>> 1. We can access to the foreign table even during recovery in the HEAD.
>>>>>>>> But in the patched version, when I did that, I got the following error.
>>>>>>>> Is this intentional?
>>>>>>>>
>>>>>>>> ERROR:  cannot assign TransactionIds during recovery
>>>>>>>
>>>>>>> No, it should be fixed. I'm going to fix this by not collecting
>>>>>>> participants for atomic commit during recovery.
>>>>>>
>>>>>> Thanks for trying to fix the issues!
>>>>>>
>>>>>> I'd like to report one more issue. When I started new transaction
>>>>>> in the local server, executed INSERT in the remote server via
>>>>>> postgres_fdw and then quit psql, I got the following assertion failure.
>>>>>>
>>>>>> TRAP: FailedAssertion("fdwxact", File: "fdwxact.c", Line: 1570)
>>>>>> 0   postgres                            0x000000010d52f3c0 ExceptionalCondition + 160
>>>>>> 1   postgres                            0x000000010cefbc49 ForgetAllFdwXactParticipants + 313
>>>>>> 2   postgres                            0x000000010cefff14 AtProcExit_FdwXact + 20
>>>>>> 3   postgres                            0x000000010d313fe3 shmem_exit + 179
>>>>>> 4   postgres                            0x000000010d313e7a proc_exit_prepare + 122
>>>>>> 5   postgres                            0x000000010d313da3 proc_exit + 19
>>>>>> 6   postgres                            0x000000010d35112f PostgresMain + 3711
>>>>>> 7   postgres                            0x000000010d27bb3a BackendRun + 570
>>>>>> 8   postgres                            0x000000010d27af6b BackendStartup + 475
>>>>>> 9   postgres                            0x000000010d279ed1 ServerLoop + 593
>>>>>> 10  postgres                            0x000000010d277940 PostmasterMain + 6016
>>>>>> 11  postgres                            0x000000010d1597b9 main + 761
>>>>>> 12  libdyld.dylib                       0x00007fff7161e3d5 start + 1
>>>>>> 13  ???                                 0x0000000000000003 0x0 + 3
>>>>>>
>>>>>
>>>>> Thank you for reporting the issue!
>>>>>
>>>>> I've attached the latest version patch that incorporated all comments
>>>>> I got so far. I've removed the patch adding the 'prefer' mode of
>>>>> foreign_twophase_commit to keep the patch set simple.
>>>>
>>>>
>>>> I have started to review the patchset. Just a quick comment.
>>>>
>>>> Patch v24-0002-Support-atomic-commit-among-multiple-foreign-ser.patch
>>>> contains changes (adding fdwxact includes) for
>>>> src/backend/executor/nodeForeignscan.c,  src/backend/executor/nodeModifyTable.c
>>>> and  src/backend/executor/execPartition.c files that doesn't seem to be
>>>> required with the latest version.
>>>
>>> Thanks for your comment.
>>>
>>> Right. I've removed these changes on the local branch.
>>
>> The latest patches failed to be applied to the master branch. Could you rebase the patches?
>>
>
> Thank you for letting me know. I've attached the latest version patch set.

Thanks for updating the patch!

IMO it's not easy to commit this 2PC patch at once because it's still large
and complicated. So I'm thinking it's better to separate the feature into
several parts and commit them gradually. What about separating
the feature into the following parts?

#1
Originally the server just executed xact callback that each FDW registered
when the transaction was committed. The patch changes this so that
the server manages the participants of FDW in the transaction and triggers
them to execute COMMIT or ROLLBACK. IMO this change can be applied
without 2PC feature. Thought?

Even if we commit this patch and add new interface for FDW, we would
need to keep the old interface, for the FDW providing only old interface.


#2
Originally when there was the FDW access in the transaction,
PREPARE TRANSACTION on that transaction failed with an error. The patch
allows PREPARE TRANSACTION and COMMIT/ROLLBACK PREPARED
even when FDW access occurs in the transaction. IMO this change can be
applied without *automatic* 2PC feature (i.e., PREPARE TRANSACTION and
COMMIT/ROLLBACK PREPARED are automatically executed for each FDW
inside "top" COMMIT command). Thought?

I'm not sure yet whether automatic resolution of "unresolved" prepared
transactions by the resolver process is necessary for this change or not.
If it's not necessary, it's better to exclude the resolver process from this
change, at this stage, to make the patch simpler.


#3
Finally IMO we can provide the patch supporting "automatic" 2PC for each FDW,
based on the #1 and #2 patches.


What's your opinion about this?

Regards,

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


Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

Fujii Masao-4


On 2020/09/07 17:59, Fujii Masao wrote:

>
>
> On 2020/08/21 15:25, Masahiko Sawada wrote:
>> On Fri, 21 Aug 2020 at 00:36, Fujii Masao <[hidden email]> wrote:
>>>
>>>
>>>
>>> On 2020/07/27 15:59, Masahiko Sawada wrote:
>>>> On Thu, 23 Jul 2020 at 22:51, Muhammad Usama <[hidden email]> wrote:
>>>>>
>>>>>
>>>>>
>>>>> On Wed, Jul 22, 2020 at 12:42 PM Masahiko Sawada <[hidden email]> wrote:
>>>>>>
>>>>>> On Sat, 18 Jul 2020 at 01:55, Fujii Masao <[hidden email]> wrote:
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>> On 2020/07/16 14:47, Masahiko Sawada wrote:
>>>>>>>> On Tue, 14 Jul 2020 at 11:19, Fujii Masao <[hidden email]> wrote:
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> On 2020/07/14 9:08, Masahiro Ikeda wrote:
>>>>>>>>>>> I've attached the latest version patches. I've incorporated the review
>>>>>>>>>>> comments I got so far and improved locking strategy.
>>>>>>>>>>
>>>>>>>>>> Thanks for updating the patch!
>>>>>>>>>
>>>>>>>>> +1
>>>>>>>>> I'm interested in these patches and now studying them. While checking
>>>>>>>>> the behaviors of the patched PostgreSQL, I got three comments.
>>>>>>>>
>>>>>>>> Thank you for testing this patch!
>>>>>>>>
>>>>>>>>>
>>>>>>>>> 1. We can access to the foreign table even during recovery in the HEAD.
>>>>>>>>> But in the patched version, when I did that, I got the following error.
>>>>>>>>> Is this intentional?
>>>>>>>>>
>>>>>>>>> ERROR:  cannot assign TransactionIds during recovery
>>>>>>>>
>>>>>>>> No, it should be fixed. I'm going to fix this by not collecting
>>>>>>>> participants for atomic commit during recovery.
>>>>>>>
>>>>>>> Thanks for trying to fix the issues!
>>>>>>>
>>>>>>> I'd like to report one more issue. When I started new transaction
>>>>>>> in the local server, executed INSERT in the remote server via
>>>>>>> postgres_fdw and then quit psql, I got the following assertion failure.
>>>>>>>
>>>>>>> TRAP: FailedAssertion("fdwxact", File: "fdwxact.c", Line: 1570)
>>>>>>> 0   postgres                            0x000000010d52f3c0 ExceptionalCondition + 160
>>>>>>> 1   postgres                            0x000000010cefbc49 ForgetAllFdwXactParticipants + 313
>>>>>>> 2   postgres                            0x000000010cefff14 AtProcExit_FdwXact + 20
>>>>>>> 3   postgres                            0x000000010d313fe3 shmem_exit + 179
>>>>>>> 4   postgres                            0x000000010d313e7a proc_exit_prepare + 122
>>>>>>> 5   postgres                            0x000000010d313da3 proc_exit + 19
>>>>>>> 6   postgres                            0x000000010d35112f PostgresMain + 3711
>>>>>>> 7   postgres                            0x000000010d27bb3a BackendRun + 570
>>>>>>> 8   postgres                            0x000000010d27af6b BackendStartup + 475
>>>>>>> 9   postgres                            0x000000010d279ed1 ServerLoop + 593
>>>>>>> 10  postgres                            0x000000010d277940 PostmasterMain + 6016
>>>>>>> 11  postgres                            0x000000010d1597b9 main + 761
>>>>>>> 12  libdyld.dylib                       0x00007fff7161e3d5 start + 1
>>>>>>> 13  ???                                 0x0000000000000003 0x0 + 3
>>>>>>>
>>>>>>
>>>>>> Thank you for reporting the issue!
>>>>>>
>>>>>> I've attached the latest version patch that incorporated all comments
>>>>>> I got so far. I've removed the patch adding the 'prefer' mode of
>>>>>> foreign_twophase_commit to keep the patch set simple.
>>>>>
>>>>>
>>>>> I have started to review the patchset. Just a quick comment.
>>>>>
>>>>> Patch v24-0002-Support-atomic-commit-among-multiple-foreign-ser.patch
>>>>> contains changes (adding fdwxact includes) for
>>>>> src/backend/executor/nodeForeignscan.c,  src/backend/executor/nodeModifyTable.c
>>>>> and  src/backend/executor/execPartition.c files that doesn't seem to be
>>>>> required with the latest version.
>>>>
>>>> Thanks for your comment.
>>>>
>>>> Right. I've removed these changes on the local branch.
>>>
>>> The latest patches failed to be applied to the master branch. Could you rebase the patches?
>>>
>>
>> Thank you for letting me know. I've attached the latest version patch set.
>
> Thanks for updating the patch!
>
> IMO it's not easy to commit this 2PC patch at once because it's still large
> and complicated. So I'm thinking it's better to separate the feature into
> several parts and commit them gradually. What about separating
> the feature into the following parts?
>
> #1
> Originally the server just executed xact callback that each FDW registered
> when the transaction was committed. The patch changes this so that
> the server manages the participants of FDW in the transaction and triggers
> them to execute COMMIT or ROLLBACK. IMO this change can be applied
> without 2PC feature. Thought?
>
> Even if we commit this patch and add new interface for FDW, we would
> need to keep the old interface, for the FDW providing only old interface.
>
>
> #2
> Originally when there was the FDW access in the transaction,
> PREPARE TRANSACTION on that transaction failed with an error. The patch
> allows PREPARE TRANSACTION and COMMIT/ROLLBACK PREPARED
> even when FDW access occurs in the transaction. IMO this change can be
> applied without *automatic* 2PC feature (i.e., PREPARE TRANSACTION and
> COMMIT/ROLLBACK PREPARED are automatically executed for each FDW
> inside "top" COMMIT command). Thought?
>
> I'm not sure yet whether automatic resolution of "unresolved" prepared
> transactions by the resolver process is necessary for this change or not.
> If it's not necessary, it's better to exclude the resolver process from this
> change, at this stage, to make the patch simpler.
>
>
> #3
> Finally IMO we can provide the patch supporting "automatic" 2PC for each FDW,
> based on the #1 and #2 patches.
>
>
> What's your opinion about this?

Also I'd like to report some typos in the patch.

+#define ServerSupportTransactionCallack(fdw_part) \

"Callack" in this macro name should be "Callback"?

+#define SeverSupportTwophaseCommit(fdw_part) \

"Sever" in this macro name should be "Server"?

+  proname => 'pg_stop_foreing_xact_resolver', provolatile => 'v', prorettype => 'bool',

"foreing" should be "foreign"?

+ * FdwXact entry we call get_preparedid callback to get a transaction

"get_preparedid" should be "get_prepareid"?

Regards,

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


Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

akapila
In reply to this post by Fujii Masao-4
On Mon, Sep 7, 2020 at 2:29 PM Fujii Masao <[hidden email]> wrote:
>
> IMO it's not easy to commit this 2PC patch at once because it's still large
> and complicated. So I'm thinking it's better to separate the feature into
> several parts and commit them gradually.
>

Hmm, I don't see that we have a consensus on the design and or
interfaces of this patch and without that proceeding for commit
doesn't seem advisable. Here are a few points which I remember offhand
that require more work.
1. There is a competing design proposed and being discussed in another
thread [1] for this purpose. I think both the approaches have pros and
cons but there doesn't seem to be any conclusion yet on which one is
better.
2. In this thread, we have discussed to try integrating this patch
with some other FDWs (say MySQL, mongodb, etc.) to ensure that the
APIs we are exposing are general enough that other FDWs can use them
to implement 2PC. I could see some speculations about the same but no
concrete work on the same has been done.
3. In another thread [1], we have seen that the patch being discussed
in this thread might need to re-designed if we have to use some other
design for global-visibility than what is proposed in that thread. I
think it is quite likely that can happen considering no one is able to
come up with the solution to major design problems spotted in that
patch yet.

It appears to me that even though these points were raised before in
some form we are just trying to bypass them to commit whatever we have
in the current patch which I find quite surprising.

[1] - https://www.postgresql.org/message-id/07b2c899-4ed0-4c87-1327-23c750311248%40postgrespro.ru

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

Fujii Masao-4


On 2020/09/08 10:34, Amit Kapila wrote:

> On Mon, Sep 7, 2020 at 2:29 PM Fujii Masao <[hidden email]> wrote:
>>
>> IMO it's not easy to commit this 2PC patch at once because it's still large
>> and complicated. So I'm thinking it's better to separate the feature into
>> several parts and commit them gradually.
>>
>
> Hmm, I don't see that we have a consensus on the design and or
> interfaces of this patch and without that proceeding for commit
> doesn't seem advisable. Here are a few points which I remember offhand
> that require more work.

Thanks!

> 1. There is a competing design proposed and being discussed in another
> thread [1] for this purpose. I think both the approaches have pros and
> cons but there doesn't seem to be any conclusion yet on which one is
> better.

I was thinking that [1] was discussing global snapshot feature for
"atomic visibility" rather than the solution like 2PC for "atomic commit".
But if another approach for "atomic commit" was also proposed at [1],
that's good. I will check that.

> 2. In this thread, we have discussed to try integrating this patch
> with some other FDWs (say MySQL, mongodb, etc.) to ensure that the
> APIs we are exposing are general enough that other FDWs can use them
> to implement 2PC. I could see some speculations about the same but no
> concrete work on the same has been done.

Yes, you're right.

> 3. In another thread [1], we have seen that the patch being discussed
> in this thread might need to re-designed if we have to use some other
> design for global-visibility than what is proposed in that thread. I
> think it is quite likely that can happen considering no one is able to
> come up with the solution to major design problems spotted in that
> patch yet.

You imply that global-visibility patch should be come first before "2PC" patch?

Regards,

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


Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

akapila
On Tue, Sep 8, 2020 at 8:05 AM Fujii Masao <[hidden email]> wrote:

>
> On 2020/09/08 10:34, Amit Kapila wrote:
> > On Mon, Sep 7, 2020 at 2:29 PM Fujii Masao <[hidden email]> wrote:
> >>
> >> IMO it's not easy to commit this 2PC patch at once because it's still large
> >> and complicated. So I'm thinking it's better to separate the feature into
> >> several parts and commit them gradually.
> >>
> >
> > Hmm, I don't see that we have a consensus on the design and or
> > interfaces of this patch and without that proceeding for commit
> > doesn't seem advisable. Here are a few points which I remember offhand
> > that require more work.
>
> Thanks!
>
> > 1. There is a competing design proposed and being discussed in another
> > thread [1] for this purpose. I think both the approaches have pros and
> > cons but there doesn't seem to be any conclusion yet on which one is
> > better.
>
> I was thinking that [1] was discussing global snapshot feature for
> "atomic visibility" rather than the solution like 2PC for "atomic commit".
> But if another approach for "atomic commit" was also proposed at [1],
> that's good. I will check that.
>

Okay, that makes sense.

> > 2. In this thread, we have discussed to try integrating this patch
> > with some other FDWs (say MySQL, mongodb, etc.) to ensure that the
> > APIs we are exposing are general enough that other FDWs can use them
> > to implement 2PC. I could see some speculations about the same but no
> > concrete work on the same has been done.
>
> Yes, you're right.
>
> > 3. In another thread [1], we have seen that the patch being discussed
> > in this thread might need to re-designed if we have to use some other
> > design for global-visibility than what is proposed in that thread. I
> > think it is quite likely that can happen considering no one is able to
> > come up with the solution to major design problems spotted in that
> > patch yet.
>
> You imply that global-visibility patch should be come first before "2PC" patch?
>

I intend to say that the global-visibility work can impact this in a
major way and we have analyzed that to some extent during a discussion
on the other thread. So, I think without having a complete
design/solution that addresses both the 2PC and global-visibility, it
is not apparent what is the right way to proceed. It seems to me that
rather than working on individual (or smaller) parts one needs to come
up with a bigger picture (or overall design) and then once we have
figured that out correctly, it would be easier to decide which parts
can go first.

--
With Regards,
Amit Kapila.


Reply | Threaded
Open this post in threaded view
|

RE: Transactions involving multiple postgres foreign servers, take 2

tsunakawa.takay@fujitsu.com
From: Amit Kapila <[hidden email]>
> I intend to say that the global-visibility work can impact this in a
> major way and we have analyzed that to some extent during a discussion
> on the other thread. So, I think without having a complete
> design/solution that addresses both the 2PC and global-visibility, it
> is not apparent what is the right way to proceed. It seems to me that
> rather than working on individual (or smaller) parts one needs to come
> up with a bigger picture (or overall design) and then once we have
> figured that out correctly, it would be easier to decide which parts
> can go first.

I'm really sorry I've been getting late and late and latex10 to publish the revised scale-out design wiki to discuss the big picture!  I don't know why I'm taking this long time; I feel I were captive in a time prison (yes, nobody is holding me captive; I'm just late.)  Please wait a few days.

But to proceed with the development, let me comment on the atomic commit and global visibility.

* We have to hear from Andrey about their check on the possibility that Clock-SI could be Microsoft's patent and if we can avoid it.

* I have a feeling that we can adopt the algorithm used by Spanner, CockroachDB, and YugabyteDB.  That is, 2PC for multi-node atomic commit, Paxos or Raft for replica synchronization (in the process of commit) to make 2PC more highly available, and the timestamp-based global visibility.  However, the timestamp-based approach makes the database instance shut down when the node's clock is distant from the other nodes.

* Or, maybe we can use the following Commitment ordering that doesn't require the timestamp or any other information to be transferred among the cluster nodes.  However, this seems to have to track the order of read and write operations among concurrent transactions to ensure the correct commit order, so I'm not sure about the performance.  The MVCO paper seems to present the information we need, but I haven't understood it well yet (it's difficult.)  Could you anybody kindly interpret this?

Commitment ordering (CO) - yoavraz2
https://sites.google.com/site/yoavraz2/the_principle_of_co


As for the Sawada-san's 2PC patch, which I find interesting purely as FDW enhancement, I raised the following issues to be addressed:

1. Make FDW API implementable by other FDWs than postgres_fdw (this is what Amit-san kindly pointed out.)  I think oracle_fdw and jdbc_fdw would be good examples to consider, while MySQL may not be good because it exposes the XA feature as SQL statements, not C functions as defined in the XA specification.

2. 2PC processing is queued and serialized in one background worker.  That severely subdues transaction throughput.  Each backend should perform 2PC.

3. postgres_fdw cannot detect remote updates when the UDF executed on a remote node updates data.


Regards
Takayuki Tsunakawa



Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

Masahiko Sawada-2
In reply to this post by Fujii Masao-4
On Mon, 7 Sep 2020 at 17:59, Fujii Masao <[hidden email]> wrote:

>
>
>
> On 2020/08/21 15:25, Masahiko Sawada wrote:
> > On Fri, 21 Aug 2020 at 00:36, Fujii Masao <[hidden email]> wrote:
> >>
> >>
> >>
> >> On 2020/07/27 15:59, Masahiko Sawada wrote:
> >>> On Thu, 23 Jul 2020 at 22:51, Muhammad Usama <[hidden email]> wrote:
> >>>>
> >>>>
> >>>>
> >>>> On Wed, Jul 22, 2020 at 12:42 PM Masahiko Sawada <[hidden email]> wrote:
> >>>>>
> >>>>> On Sat, 18 Jul 2020 at 01:55, Fujii Masao <[hidden email]> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> On 2020/07/16 14:47, Masahiko Sawada wrote:
> >>>>>>> On Tue, 14 Jul 2020 at 11:19, Fujii Masao <[hidden email]> wrote:
> >>>>>>>>
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 2020/07/14 9:08, Masahiro Ikeda wrote:
> >>>>>>>>>> I've attached the latest version patches. I've incorporated the review
> >>>>>>>>>> comments I got so far and improved locking strategy.
> >>>>>>>>>
> >>>>>>>>> Thanks for updating the patch!
> >>>>>>>>
> >>>>>>>> +1
> >>>>>>>> I'm interested in these patches and now studying them. While checking
> >>>>>>>> the behaviors of the patched PostgreSQL, I got three comments.
> >>>>>>>
> >>>>>>> Thank you for testing this patch!
> >>>>>>>
> >>>>>>>>
> >>>>>>>> 1. We can access to the foreign table even during recovery in the HEAD.
> >>>>>>>> But in the patched version, when I did that, I got the following error.
> >>>>>>>> Is this intentional?
> >>>>>>>>
> >>>>>>>> ERROR:  cannot assign TransactionIds during recovery
> >>>>>>>
> >>>>>>> No, it should be fixed. I'm going to fix this by not collecting
> >>>>>>> participants for atomic commit during recovery.
> >>>>>>
> >>>>>> Thanks for trying to fix the issues!
> >>>>>>
> >>>>>> I'd like to report one more issue. When I started new transaction
> >>>>>> in the local server, executed INSERT in the remote server via
> >>>>>> postgres_fdw and then quit psql, I got the following assertion failure.
> >>>>>>
> >>>>>> TRAP: FailedAssertion("fdwxact", File: "fdwxact.c", Line: 1570)
> >>>>>> 0   postgres                            0x000000010d52f3c0 ExceptionalCondition + 160
> >>>>>> 1   postgres                            0x000000010cefbc49 ForgetAllFdwXactParticipants + 313
> >>>>>> 2   postgres                            0x000000010cefff14 AtProcExit_FdwXact + 20
> >>>>>> 3   postgres                            0x000000010d313fe3 shmem_exit + 179
> >>>>>> 4   postgres                            0x000000010d313e7a proc_exit_prepare + 122
> >>>>>> 5   postgres                            0x000000010d313da3 proc_exit + 19
> >>>>>> 6   postgres                            0x000000010d35112f PostgresMain + 3711
> >>>>>> 7   postgres                            0x000000010d27bb3a BackendRun + 570
> >>>>>> 8   postgres                            0x000000010d27af6b BackendStartup + 475
> >>>>>> 9   postgres                            0x000000010d279ed1 ServerLoop + 593
> >>>>>> 10  postgres                            0x000000010d277940 PostmasterMain + 6016
> >>>>>> 11  postgres                            0x000000010d1597b9 main + 761
> >>>>>> 12  libdyld.dylib                       0x00007fff7161e3d5 start + 1
> >>>>>> 13  ???                                 0x0000000000000003 0x0 + 3
> >>>>>>
> >>>>>
> >>>>> Thank you for reporting the issue!
> >>>>>
> >>>>> I've attached the latest version patch that incorporated all comments
> >>>>> I got so far. I've removed the patch adding the 'prefer' mode of
> >>>>> foreign_twophase_commit to keep the patch set simple.
> >>>>
> >>>>
> >>>> I have started to review the patchset. Just a quick comment.
> >>>>
> >>>> Patch v24-0002-Support-atomic-commit-among-multiple-foreign-ser.patch
> >>>> contains changes (adding fdwxact includes) for
> >>>> src/backend/executor/nodeForeignscan.c,  src/backend/executor/nodeModifyTable.c
> >>>> and  src/backend/executor/execPartition.c files that doesn't seem to be
> >>>> required with the latest version.
> >>>
> >>> Thanks for your comment.
> >>>
> >>> Right. I've removed these changes on the local branch.
> >>
> >> The latest patches failed to be applied to the master branch. Could you rebase the patches?
> >>
> >
> > Thank you for letting me know. I've attached the latest version patch set.
>
> Thanks for updating the patch!
>
> IMO it's not easy to commit this 2PC patch at once because it's still large
> and complicated. So I'm thinking it's better to separate the feature into
> several parts and commit them gradually. What about separating
> the feature into the following parts?
>
> #1
> Originally the server just executed xact callback that each FDW registered
> when the transaction was committed. The patch changes this so that
> the server manages the participants of FDW in the transaction and triggers
> them to execute COMMIT or ROLLBACK. IMO this change can be applied
> without 2PC feature. Thought?
>
> Even if we commit this patch and add new interface for FDW, we would
> need to keep the old interface, for the FDW providing only old interface.
>
>
> #2
> Originally when there was the FDW access in the transaction,
> PREPARE TRANSACTION on that transaction failed with an error. The patch
> allows PREPARE TRANSACTION and COMMIT/ROLLBACK PREPARED
> even when FDW access occurs in the transaction. IMO this change can be
> applied without *automatic* 2PC feature (i.e., PREPARE TRANSACTION and
> COMMIT/ROLLBACK PREPARED are automatically executed for each FDW
> inside "top" COMMIT command). Thought?
>
> I'm not sure yet whether automatic resolution of "unresolved" prepared
> transactions by the resolver process is necessary for this change or not.
> If it's not necessary, it's better to exclude the resolver process from this
> change, at this stage, to make the patch simpler.
>
>
> #3
> Finally IMO we can provide the patch supporting "automatic" 2PC for each FDW,
> based on the #1 and #2 patches.
>
>
> What's your opinion about this?

Regardless of which approaches of 2PC implementation being selected
splitting the patch into logical small patches is a good idea and the
above suggestion makes sense to me.

Regarding #2, I guess that we would need resolver and launcher
processes even if we would support only manual PREPARE TRANSACTION and
COMMIT/ROLLBACK PREPARED commands:

On COMMIT PREPARED command, I think we should commit the local
prepared transaction first then commit foreign prepared transactions.
Otherwise, it violates atomic commit principles when the local node
failed to commit a foreign prepared transaction and the user changed
to ROLLBACK PREPARED. OTOH once we committed locally, we cannot change
to rollback. And attempting to commit foreign prepared transactions
could lead an error due to connection error, OOM caused by palloc etc.
Therefore we discussed using background processes, resolver and
launcher, to take in charge of committing foreign prepared
transactions so that the process who executed COMMIT PREPARED will
never  error out after local commit. So I think the patch #2 will have
the patch also adding resolver and launcher processes. And in the
patch #3 we will change the code to support automatic 2PC as you
suggested.

In addition, the part of the automatic resolution of in-doubt
transactions can also be a separate patch, which will be the #4 patch.

Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

Ashutosh Bapat-2
In reply to this post by Fujii Masao-4
On Mon, Sep 7, 2020 at 2:29 PM Fujii Masao <[hidden email]> wrote:

>
> #2
> Originally when there was the FDW access in the transaction,
> PREPARE TRANSACTION on that transaction failed with an error. The patch
> allows PREPARE TRANSACTION and COMMIT/ROLLBACK PREPARED
> even when FDW access occurs in the transaction. IMO this change can be
> applied without *automatic* 2PC feature (i.e., PREPARE TRANSACTION and
> COMMIT/ROLLBACK PREPARED are automatically executed for each FDW
> inside "top" COMMIT command). Thought?
>
> I'm not sure yet whether automatic resolution of "unresolved" prepared
> transactions by the resolver process is necessary for this change or not.
> If it's not necessary, it's better to exclude the resolver process from this
> change, at this stage, to make the patch simpler.

I agree with this. However, in case of explicit prepare, if we are not
going to try automatic resolution, it might be better to provide a way
to pass the information about transactions prepared on the foreign
servers if they can not be resolved at the time of commit so that the
user can take it up to resolve those him/herself. This was an idea
that Tom had suggested at the very beginning of the first take.

--
Best Wishes,
Ashutosh Bapat


Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

Fujii Masao-4
In reply to this post by akapila


On 2020/09/08 12:03, Amit Kapila wrote:

> On Tue, Sep 8, 2020 at 8:05 AM Fujii Masao <[hidden email]> wrote:
>>
>> On 2020/09/08 10:34, Amit Kapila wrote:
>>> On Mon, Sep 7, 2020 at 2:29 PM Fujii Masao <[hidden email]> wrote:
>>>>
>>>> IMO it's not easy to commit this 2PC patch at once because it's still large
>>>> and complicated. So I'm thinking it's better to separate the feature into
>>>> several parts and commit them gradually.
>>>>
>>>
>>> Hmm, I don't see that we have a consensus on the design and or
>>> interfaces of this patch and without that proceeding for commit
>>> doesn't seem advisable. Here are a few points which I remember offhand
>>> that require more work.
>>
>> Thanks!
>>
>>> 1. There is a competing design proposed and being discussed in another
>>> thread [1] for this purpose. I think both the approaches have pros and
>>> cons but there doesn't seem to be any conclusion yet on which one is
>>> better.
>>
>> I was thinking that [1] was discussing global snapshot feature for
>> "atomic visibility" rather than the solution like 2PC for "atomic commit".
>> But if another approach for "atomic commit" was also proposed at [1],
>> that's good. I will check that.
>>
>
> Okay, that makes sense.

I read Alexey's 2PC patch (0001-Add-postgres_fdw.use_twophase-GUC-to-use-2PC.patch)
proposed at [1]. As Alexey told at that thread, there are two big differences
between his patch and Sawada-san's; 1) whether there is the resolver process
for foreign transactions, 2) 2PC logic is implemented only inside postgres_fdw
or both FDW and PostgreSQL core.

I think that 2) is the first decision point. Alexey's 2PC patch is very simple
and all the 2PC logic is implemented only inside postgres_fdw. But this
means that 2PC is not usable if multiple types of FDW (e.g., postgres_fdw
and mysql_fdw) participate at the transaction. This may be ok if we implement
2PC feature only for PostgreSQL sharding using postgres_fdw. But if we
implement 2PC as the improvement on FDW independently from PostgreSQL
sharding, I think that it's necessary to support other FDW. And this is our
direction, isn't it?

Sawada-san's patch supports that case by implememnting some conponents
for that also in PostgreSQL core. For example, with the patch, all the remote
transactions that participate at the transaction are managed by PostgreSQL
core instead of postgres_fdw layer.

Therefore, at least regarding the difference 2), I think that Sawada-san's
approach is better. Thought?

[1]
https://postgr.es/m/3ef7877bfed0582019eab3d462a43275@...

Regards,

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


Reply | Threaded
Open this post in threaded view
|

RE: Transactions involving multiple postgres foreign servers, take 2

tsunakawa.takay@fujitsu.com
Alexey-san, Sawada-san,
cc: Fujii-san,


From: Fujii Masao <[hidden email]>
> But if we
> implement 2PC as the improvement on FDW independently from PostgreSQL
> sharding, I think that it's necessary to support other FDW. And this is our
> direction, isn't it?

I understand the same way as Fujii san.  2PC FDW is itself useful, so I think we should pursue the tidy FDW interface and good performance withinn the FDW framework.  "tidy" means that many other FDWs should be able to implement it.  I guess XA/JTA is the only material we can use to consider whether the FDW interface is good.


> Sawada-san's patch supports that case by implememnting some conponents
> for that also in PostgreSQL core. For example, with the patch, all the remote
> transactions that participate at the transaction are managed by PostgreSQL
> core instead of postgres_fdw layer.
>
> Therefore, at least regarding the difference 2), I think that Sawada-san's
> approach is better. Thought?

I think so.  Sawada-san's patch needs to address the design issues I posed before digging into the code for thorough review, though.

BTW, is there something Sawada-san can take from Alexey-san's patch?  I'm concerned about the performance for practical use.  Do you two have differences in these points, for instance?  The first two items are often cited to evaluate the algorithm's performance, as you know.

* The number of round trips to remote nodes.
* The number of disk I/Os on each node and all nodes in total (WAL, two-phase file, pg_subtrans file, CLOG?).
* Are prepare and commit executed in parallel on remote nodes? (serious DBMSs do so)
* Is there any serialization point in the processing? (Sawada-san's has one)

I'm sorry to repeat myself, but I don't think we can compromise the 2PC performance.  Of course, we recommend users to design a schema that co-locates data that each transaction accesses to avoid 2PC, but it's not always possible (e.g., when secondary indexes are used.)

Plus, as the following quote from TPC-C specification shows, TPC-C requires 15% of (Payment?) transactions to do 2PC.  (I knew this on Microsoft, CockroachDB, or Citus Data's site.)


--------------------------------------------------
Independent of the mode of selection, the customer resident
warehouse is the home warehouse 85% of the time and is a randomly selected remote warehouse 15% of the time.
This can be implemented by generating two random numbers x and y within [1 .. 100];

. If x <= 85 a customer is selected from the selected district number (C_D_ID = D_ID) and the home warehouse
number (C_W_ID = W_ID). The customer is paying through his/her own warehouse.

. If x > 85 a customer is selected from a random district number (C_D_ID is randomly selected within [1 .. 10]),
and a random remote warehouse number (C_W_ID is randomly selected within the range of active
warehouses (see Clause 4.2.2), and C_W_ID ≠ W_ID). The customer is paying through a warehouse and a
district other than his/her own.
--------------------------------------------------


Regards
Takayuki Tsunakawa


Reply | Threaded
Open this post in threaded view
|

Re: Transactions involving multiple postgres foreign servers, take 2

Fujii Masao-4


On 2020/09/10 10:13, [hidden email] wrote:

> Alexey-san, Sawada-san,
> cc: Fujii-san,
>
>
> From: Fujii Masao <[hidden email]>
>> But if we
>> implement 2PC as the improvement on FDW independently from PostgreSQL
>> sharding, I think that it's necessary to support other FDW. And this is our
>> direction, isn't it?
>
> I understand the same way as Fujii san.  2PC FDW is itself useful, so I think we should pursue the tidy FDW interface and good performance withinn the FDW framework.  "tidy" means that many other FDWs should be able to implement it.  I guess XA/JTA is the only material we can use to consider whether the FDW interface is good.

Originally start(), commit() and rollback() are supported as FDW interfaces. With his patch, prepare() is supported. What other interfaces need to be supported per XA/JTA?

As far as I and Sawada-san discussed this upthread, to support MySQL, another type of start() would be necessary to issue "XA START id" command. end() might be also necessary to issue "XA END id", but that command can be issued via prepare() together with "XA PREPARE id".

I'm not familiar with XA/JTA and XA transaction interfaces on other major DBMS. So I'd like to know what other interfaces are necessary additionally?

>
>
>> Sawada-san's patch supports that case by implememnting some conponents
>> for that also in PostgreSQL core. For example, with the patch, all the remote
>> transactions that participate at the transaction are managed by PostgreSQL
>> core instead of postgres_fdw layer.
>>
>> Therefore, at least regarding the difference 2), I think that Sawada-san's
>> approach is better. Thought?
>
> I think so.  Sawada-san's patch needs to address the design issues I posed before digging into the code for thorough review, though.
>
> BTW, is there something Sawada-san can take from Alexey-san's patch?  I'm concerned about the performance for practical use.  Do you two have differences in these points, for instance?

IMO Sawada-san's version of 2PC is less performant, but it's because
his patch provides more functionality. For example, with his patch,
WAL is written to automatically complete the unresolve foreign transactions
in the case of failure. OTOH, Alexey patch introduces no new WAL for 2PC.
Of course, generating more WAL would cause more overhead.
But if we need automatic resolution feature, it's inevitable to introduce
new WAL whichever the patch we choose.

Regards,

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


123456789