Re: Transactions involving multiple postgres foreign servers, take 2

On 2020/07/27 15:59, Masahiko Sawada wrote:
> 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.

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