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

On 2018-Jun-26, Arseny Sher wrote:

> Alvaro Herrera <alvherre@2ndquadrant.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