Re: [HACKERS] logical decoding of two-phase transactions

On Wed, Sep 30, 2020 at 3:27 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Wed, Sep 30, 2020 at 3:12 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> >
> > On Wed, Sep 30, 2020 at 3:08 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > >
> > > On Wed, Sep 30, 2020 at 2:46 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > >
> > > > On Wed, Sep 30, 2020 at 2:36 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > > >
> > > > > On Tue, Sep 29, 2020 at 8:04 PM Dilip Kumar <dilipbalaut@gmail.com> wrote:
> > > > > >
> > > > > > I have started looking into you latest patches,  as of now I have a
> > > > > > few comments.
> > > > > >
> > > > > > v6-0001
> > > > > >
> > > > > > @@ -1987,7 +2072,7 @@ ReorderBufferProcessTXN(ReorderBuffer *rb,
> > > > > > ReorderBufferTXN *txn,
> > > > > >   prev_lsn = change->lsn;
> > > > > >
> > > > > >   /* Set the current xid to detect concurrent aborts. */
> > > > > > - if (streaming)
> > > > > > + if (streaming || rbtxn_prepared(change->txn))
> > > > > >   {
> > > > > >   curtxn = change->txn;
> > > > > >   SetupCheckXidLive(curtxn->xid);
> > > > > > @@ -2249,7 +2334,6 @@ ReorderBufferProcessTXN(ReorderBuffer *rb,
> > > > > > ReorderBufferTXN *txn,
> > > > > >   break;
> > > > > >   }
> > > > > >   }
> > > > > > -
> > > > > >
> > > > > > For streaming transaction we need to check the xid everytime because
> > > > > > there could concurrent a subtransaction abort, but
> > > > > > for two-phase we don't need to call SetupCheckXidLive everytime,
> > > > > > because we are sure that transaction is going to be
> > > > > > the same throughout the processing.
> > > > > >
> > > > >
> > > > > While decoding transactions at 'prepare' time there could be multiple
> > > > > sub-transactions like in the case below. Won't that be impacted if we
> > > > > follow your suggestion here?
> > > > >
> > > > > postgres=# Begin;
> > > > > BEGIN
> > > > > postgres=*# insert into t1 values(1,'aaa');
> > > > > INSERT 0 1
> > > > > postgres=*# savepoint s1;
> > > > > SAVEPOINT
> > > > > postgres=*# insert into t1 values(2,'aaa');
> > > > > INSERT 0 1
> > > > > postgres=*# savepoint s2;
> > > > > SAVEPOINT
> > > > > postgres=*# insert into t1 values(3,'aaa');
> > > > > INSERT 0 1
> > > > > postgres=*# Prepare Transaction 'foo';
> > > > > PREPARE TRANSACTION
> > > >
> > > > But once we prepare the transaction, we can not rollback individual
> > > > subtransaction.
> > > >
> > >
> > > Sure but Rollback can come before prepare like in the case below which
> > > will appear as concurrent abort (assume there is some DDL which
> > > changes the table before the Rollback statement) because it has
> > > already been done by the backend and that need to be caught by this
> > > mechanism only.
> > >
> > > Begin;
> > > insert into t1 values(1,'aaa');
> > > savepoint s1;
> > > insert into t1 values(2,'aaa');
> > > savepoint s2;
> > > insert into t1 values(3,'aaa');
> > > Rollback to savepoint s2;
> > > insert into t1 values(4,'aaa');
> > > Prepare Transaction 'foo';
> >
> >
> > If we are streaming on the prepare that means we must have decoded
> > that rollback WAL which means we should have removed the
> > ReorderBufferTXN for those subxact.
> >
>
> Okay, valid point. We can avoid setting it for each sub-transaction in
> that case but OTOH even if we allow to set it there shouldn't be any
> bug.

Right, there will not be any bug, just an optimization.

-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com