Re: Bug in logical decoding with DDL and subtransactions

Okay, I spent some more time trying to rewrite these comments in the new
function so that they explain what we're doing in terms we use elsewhere
in the code, and according to my understanding of how this works. The
phrasing proposed by Bingshuai contains some terms that we don't use
anywhere else, such as "a decoding pass" and other variations thereof,
referring to the logical decoding "session". I would appreciate a
review for correctness; if it's wrong, please propose fixes as minimal
as possible (that is, not a complete rewrite).

By the way, the assurance that with this coding we will never call
bsearch() with a NULL subxact array seems a bit too trusting ... I think
we should just add the "return false" if nsubxacts == 0 in
TransactionIdInSubxactArray() just to be on the safe side. It costs
nothing. (Well, nothing compared to the cost of doing a hash lookup for
every single aborted subtransaction -- should we instead impose a cost
on ReorderBufferBuildTupleCidHash?)

/*
* Remove TUPLECID changes from a top-level transaction's tuplecid list.
*
* 'xid' is a subtransaction being aborted, and 'subxacts' is the list of
* its children -- which are, obviously, also being aborted.
*
* Unlike other types of changes, tuplecid ones are always queued on the
* toplevel transaction (see ReorderBufferAddNewTupleCids), so we must search
* for these changes by subxid. Because TIDs used by a catalog tuple written
* by a subtransaction that aborts can be reused later by a future command
* of the same transaction, we must remove these TUPLECIDs from the array,
* otherwise the tuplecid hash of the historical snapshot would be corrupt.
*
* If the xid being aborted is not known to this reorderbuffer to be a
* subxact, we return without doing anything, which is correct because there
* cannot possibly be TUPLECIDs to clean up; the topxid would've been created
* by ReorderBufferAddNewTupleCids(), and the association between the two is
* known because we're in SNAPBUILD_FULL_SNAPSHOT mode (otherwise the NEW_CID
* WAL records aren't decoded.)
*
* (This cleanup is pointless when xid is a top-level transaction. That case
* is handled instead by ReorderBufferCleanupTXN() clearing the whole list.)
*
* XXX this whole exercise is only needed because of keeping the TUPLECID list
* assigned to the toplevel transaction. Maybe it would be wiser to have a
* per-subxact list instead.
*/
void
ReorderBufferCleanupAbortedTupleCids(ReorderBuffer *rb,
TransactionId xid,
int nsubxacts,
TransactionId *subxacts)
{
ReorderBufferTXN *txn;
ReorderBufferTXN *toptxn;
dlist_mutable_iter it;

/*
* Search the passed-in xids for the first known subtransaction, and use
* it to find the toplevel transaction.
*/
txn = ReorderBufferTXNByXid(rb, xid, false, NULL, InvalidXLogRecPtr, false);
for (int i = 0; txn == NULL && i < nsubxacts; i++)
txn = ReorderBufferTXNByXid(rb, subxacts[i], false, NULL,
InvalidXLogRecPtr, false);

/*
* Skip when nothing usable was found. No entry at all means no NEW_CID
* record from these xids was decoded by this snapbuild so nothing was
* queued, or that we entered FULL_SNAPSHOT after the subxid was assigned
* as a child. In the latter case, we won't find the TUPLECIDs, but we
* don't care. If this is a top-level xact, this cleanup is not
* necessary.
*/
if (txn == NULL || !rbtxn_is_known_subxact(txn))
return;

/* Get the top-level transaction whose tuplecid list we're to clean */
toptxn = rbtxn_get_toptxn(txn);

dlist_foreach_modify(it, &toptxn->tuplecids)
{
ReorderBufferChange *change;

change = dlist_container(ReorderBufferChange, node, it.cur);

Assert(change->action == REORDER_BUFFER_CHANGE_INTERNAL_TUPLECID);

/*
* Remove the entry if it was recorded on behalf of the aborting
* subtransaction or one of its subcommitted children. The subxact
* array is known sorted in logical order because it comes directly
* from the xl_xact_abort WAL record, which has them sorted that way
* by construction (Xid assign order.)
*/
if (change->data.tuplecid.subxid == xid ||
TransactionIdInSubxactArray(change->data.tuplecid.subxid,
subxacts, nsubxacts))
{
dlist_delete(&change->node);
ReorderBufferFreeChange(rb, change, false);
Assert(toptxn->ntuplecids > 0);
toptxn->ntuplecids--;
}
}
}

--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"Los dioses no protegen a los insensatos. Éstos reciben protección de
otros insensatos mejor dotados" (Luis Wu, Mundo Anillo)