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

On Mon, 7 Sep 2020 at 17:59, Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
>
>
> 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?

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