From: | Peter Smith <smithpb2250(at)gmail(dot)com> |
---|---|
To: | Ajin Cherian <itsajin(at)gmail(dot)com> |
Cc: | PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: [HACKERS] logical decoding of two-phase transactions |
Date: | 2020-10-28 05:20:32 |
Message-ID: | CAHut+Pu3DBQ2r=1YoCNfN1FJdyOrYSkS3ut5vZ_wHwt+=AsLpw@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
Hi Ajin.
I have re-checked the v13 patches for how my remaining review comments
have been addressed.
On Tue, Oct 27, 2020 at 8:55 PM Ajin Cherian <itsajin(at)gmail(dot)com> wrote:
>
> > ====================
> > v12-0002. File: src/backend/replication/logical/reorderbuffer.c
> > ====================
> >
> > COMMENT
> > Line 2401
> > /*
> > * We are here due to one of the 3 scenarios:
> > * 1. As part of streaming in-progress transactions
> > * 2. Prepare of a two-phase commit
> > * 3. Commit of a transaction.
> > *
> > * If we are streaming the in-progress transaction then discard the
> > * changes that we just streamed, and mark the transactions as
> > * streamed (if they contained changes), set prepared flag as false.
> > * If part of a prepare of a two-phase commit set the prepared flag
> > * as true so that we can discard changes and cleanup tuplecids.
> > * Otherwise, remove all the
> > * changes and deallocate the ReorderBufferTXN.
> > */
> > ~
> > The above comment is beyond my understanding. Anything you could do to
> > simplify it would be good.
> >
> > For example, when viewing this function in isolation I have never
> > understood why the streaming flag and rbtxn_prepared(txn) flag are not
> > possible to be set at the same time?
> >
> > Perhaps the code is relying on just internal knowledge of how this
> > helper function gets called? And if it is just that, then IMO there
> > really should be some Asserts in the code to give more assurance about
> > that. (Or maybe use completely different flags to represent those 3
> > scenarios instead of bending the meanings of the existing flags)
> >
>
> Left this for now, probably re-look at this at a later review.
> But just to explain; this function is what does the main decoding of
> changes of a transaction.
> At what point this decoding happens is what this feature and the
> streaming in-progress feature is about. As of PG13, this decoding only
> happens at commit time. With the streaming of in-progress txn feature,
> this began to happen (if streaming enabled) at the time when the
> memory limit for decoding transactions was crossed. This 2PC feature
> is supporting decoding at the time of a PREPARE transaction.
> Now, if streaming is enabled and streaming has started as a result of
> crossing the memory threshold, then there is no need to
> again begin streaming at a PREPARE transaction as the transaction that
> is being prepared has already been streamed. Which is why this
> function will not be called when a streaming transaction is prepared
> as part of a two-phase commit.
AFAIK the last remaining issue now is only about the complexity of the
aforementioned code/comment. If you want to defer changing that until
we can come up with something better, then that is OK by me.
Apart from that I have no other pending review comments at this time.
Kind Regards,
Peter Smith.
Fujitsu Australia
From | Date | Subject | |
---|---|---|---|
Next Message | Michael Paquier | 2020-10-28 05:26:27 | Re: duplicate function oid symbols |
Previous Message | Michael Paquier | 2020-10-28 05:14:55 | Re: Add important info about ANALYZE after create Functional Index |