Hi,
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 ||
ctx->fast_forward)
return;
> 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.
Greetings,
Andres Freund