Re: Transactions involving multiple postgres foreign servers, take 2 - Mailing list pgsql-hackers

On 2020/08/21 15:25, Masahiko Sawada wrote:
> On Fri, 21 Aug 2020 at 00:36, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>
>>
>>
>> On 2020/07/27 15:59, Masahiko Sawada wrote:
>>> On Thu, 23 Jul 2020 at 22:51, Muhammad Usama <m.usama@gmail.com> wrote:
>>>>
>>>>
>>>>
>>>> On Wed, Jul 22, 2020 at 12:42 PM Masahiko Sawada <masahiko.sawada@2ndquadrant.com> wrote:
>>>>>
>>>>> On Sat, 18 Jul 2020 at 01:55, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>>>>>>
>>>>>>
>>>>>>
>>>>>> On 2020/07/16 14:47, Masahiko Sawada wrote:
>>>>>>> On Tue, 14 Jul 2020 at 11:19, Fujii Masao <masao.fujii@oss.nttdata.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?
>>
> 
> 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