Re: Transactions involving multiple postgres foreign servers, take 2

On Thu, 23 Jul 2020 at 22:51, Muhammad Usama <m(dot)usama(at)gmail(dot)com> wrote:
>
>
>
> On Wed, Jul 22, 2020 at 12:42 PM Masahiko Sawada <masahiko(dot)sawada(at)2ndquadrant(dot)com> wrote:
>>
>> On Sat, 18 Jul 2020 at 01:55, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>> >
>> >
>> >
>> > On 2020/07/16 14:47, Masahiko Sawada wrote:
>> > > On Tue, 14 Jul 2020 at 11:19, Fujii Masao <masao(dot)fujii(at)oss(dot)nttdata(dot)com> 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