Re: Transactions involving multiple postgres foreign servers, take 2

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 <masao(dot)fujii(at)oss(dot)nttdata(dot)com> wrote:
>>>
>>>
>>>
>>> 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?
>>>
>>
>> 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