Re: Fix slot's xmin advancement and subxact's lost snapshots in decoding.

On 2018-Jun-26, Arseny Sher wrote:

> Alvaro Herrera <alvherre(at)2ndquadrant(dot)com> writes:

> > * I'm a bit unsure about the comment atop ReorderBufferSetBaseSnapshot.
> > Obviously, the bit within the #if 0/#endif I'm going to remove before
> > push.
>
> It looks like you've started editing that bit and didn't finish.

Yeah, I left those lines in place as a reminder that I need to finish
editing, wondering if there's any nuance I'm missing that I need to add
to the final version.

> > I don't understand why it says "Needs to be called before any
> > changes are added with ReorderBufferQueueChange"; but if you edit that
> > function and add an assert that the base snapshot is set, it crashes
> > pretty quickly in the test_decoding tests. (The supposedly bogus
> > comment was there before your patch -- I'm not saying your comment
> > addition is faulty.)
>
> That's because REORDER_BUFFER_CHANGE_INTERNAL_COMMAND_ID changes are
> queued whenever we have read that xact has modified the catalog,
> regardless of base snapshot existence. Even if there are no changes yet,
> we will need it for correct visibility of catalog, so I am inclined to
> remove the assert and comment or rephrase the latter with 'any
> *data-carrying* changes'.

I'm struggling with this assert. I find that test_decoding passes tests
with this assertion in branch master, but fails in 9.4 - 10. Maybe I'm
running the tests wrong (no assertions in master?) but I don't see it.
It *should* fail ...

> > * I also noticed that we're doing subtxn cleanup one by one in both
> > ReorderBufferAssignChild and ReorderBufferCommitChild, which means the
> > top-level txn is sought in the hash table over and over, which seems a
> > bit silly. Not this patch's problem to fix ...
>
> There is already one-entry cache in ReorderBufferTXNByXid.

That's useless, because we use two xids: first the parent; then the
subxact. Looking up the subxact evicts the parent from the one-entry
cache, so when we loop to process next subxact, the parent is no longer
cached :-)

> We could add 'don't cache me' flag to it and use it with subxacts, or
> simply pull top-level reorderbuffer out of loops.

Yeah, but that's an API change.

> I'm fine with the rest of your edits. One more little comment:
>
> @@ -543,9 +546,10 @@ ReorderBufferQueueChange(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn,
> ReorderBufferTXN *txn;
>
> txn = ReorderBufferTXNByXid(rb, xid, true, NULL, lsn, true);
> + Assert(txn->base_snapshot != NULL || txn->is_known_as_subxact);
>
> change->lsn = lsn;
> - Assert(InvalidXLogRecPtr != lsn);
> + Assert(!XLogRecPtrIsInvalid(lsn));
> dlist_push_tail(&txn->changes, &change->node);
> txn->nentries++;
> txn->nentries_mem++;
>
> Since we do that, probably we should replace all lsn validity checks
> with XLogRectPtrIsInvalid?

I reverted this back to how it was originally. We can change it as a
style item later in all places where it appears (branch master only),
but modifying only one in a backpatched commit seems poor practice.

--
Álvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services