Re: Duplicated LSN in ReorderBuffer

From: Andres Freund <andres(at)anarazel(dot)de>
To: Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>
Cc: Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Thomas Munro <thomas(dot)munro(at)gmail(dot)com>, Ildar Musin <ildar(at)adjust(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>, Petr Jelinek <petr(at)2ndquadrant(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>
Subject: Re: Duplicated LSN in ReorderBuffer
Date: 2019-08-07 20:59:44
Views: Raw Message | Whole Thread | Download mbox | Resend email
Lists: pgsql-hackers


On 2019-08-07 16:19:13 -0400, Alvaro Herrera wrote:
> On 2019-Jul-26, Andres Freund wrote:
> > 2) We could simply assign the subtransaction to the parent using
> > ReorderBufferAssignChild() in SnapBuildProcessNewCid() or it's
> > caller. That ought to also fix the bug
> >
> > I also has the advantage that we can save some memory in transactions
> > that have some, but fewer than the ASSIGNMENT limit subtransactions,
> > because it allows us to avoid having a separate base snapshot for
> > them (c.f. ReorderBufferTransferSnapToParent()).
> I'm not sure I understood this suggestion correctly. I first tried with
> this, which seems the simplest rendition:
> --- a/src/backend/replication/logical/snapbuild.c
> +++ b/src/backend/replication/logical/snapbuild.c
> @@ -772,6 +772,12 @@ SnapBuildProcessNewCid(SnapBuild *builder, TransactionId xid,
> {
> CommandId cid;
> + if ((SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT) &&
> + (xlrec->top_xid != xid))
> + {
> + ReorderBufferAssignChild(builder->reorder, xlrec->top_xid, xid, lsn);
> + }
> +

I think we would need to do this for all values of
SnapBuildCurrentState() - after all the problem occurs because we
*previously* didn't assign subxids to the toplevel xid. Compared to the
cost of catalog changes, ReorderBufferAssignChild() is really cheap. So
I don't think there's any problem just calling it unconditionally (when
top_xid <> xid, of course).

If the above is the only change, I think the body of the if should be
unreachable, DecodeHeap2Op guards against that:

* If we don't have snapshot or we are just fast-forwarding, there is no
* point in decoding changes.
if (SnapBuildCurrentState(builder) < SNAPBUILD_FULL_SNAPSHOT ||

> I thought I would create the main txn before calling AssignChild in
> snapbuild; however, ReorderBufferTXNByXid is static in reorderbuffer.c.
> So that seems out.

There shouldn't be any need for doing that, ReorderBufferAssignChild
does that.


Andres Freund

In response to


Browse pgsql-hackers by date

  From Date Subject
Next Message Alvaro Herrera 2019-08-07 21:00:08 Re: Unused header file inclusion
Previous Message Melanie Plageman 2019-08-07 20:47:17 Re: Adding a test for speculative insert abort case