On Wed, Sep 30, 2020 at 3:12 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
>
> On Wed, Sep 30, 2020 at 3:08 PM Amit Kapila <amit.kapil...@gmail.com> wrote:
> >
> > On Wed, Sep 30, 2020 at 2:46 PM Dilip Kumar <dilipbal...@gmail.com> wrote:
> > >
> > > On Wed, Sep 30, 2020 at 2:36 PM Amit Kapila <amit.kapil...@gmail.com>
> > > wrote:
> > > >
> > > > On Tue, Sep 29, 2020 at 8:04 PM Dilip Kumar <dilipbal...@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.
--
With Regards,
Amit Kapila.
