On 2018-Jun-26, Arseny Sher wrote: > Alvaro Herrera <alvhe...@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
