Hi Hayato, On why some platforms passed: the two failing jobs are the only ones in that run built with UBSan (CFLAGS ... -fno-sanitize-recover=all -fsanitize=alignment,undefined, UBSAN_OPTIONS with abort_on_error=1). glibc's <stdlib.h> declares bsearch()'s key, base and compar __nonnull, and UBSan's nonnull-attribute check validates call arguments against that declaration even when nmemb is zero. In the glibc build I probed, the NULL/0 input returns NULL without touching base, so without the sanitizer the invalid call can go unnoticed; built with those flags, the same program reports the same "argument 2" error as the cfbot logs. With the guard, bsearch() is never reached.
The crash showed up in the stream tests rather than only the new ones: those tests reach the invalid call while scanning the toplevel's tuplecid list -- the abort has no subcommitted children, and an entry's subxid differs from the primary XID. That shape can occur in savepoint rollbacks with catalog changes. tuplecid_restart's entries match the primary XID (the bsearch is short-circuited) and tuplecid_nested has a nonempty array, so neither of them trips the NULL base. On rbtxn_is_known_subxact(): fair point. My earlier test showed that an unassociated child entry is reachable, not that the check is required for correctness. rbtxn_get_toptxn() returns the transaction itself when it has no toplevel, and tuplecid changes are always queued on the entry of the NEW_CID record's top_xid, so an unassociated child's own list is always empty -- selecting it just walks nothing, and the worst case is a missed cleanup, not a wrong removal. I compared the child-lookup loop with and without the check (leaving the primary-XID early return unchanged) on current master plus v6 and the guard, with three schedules including a mixed case that has one unassociated and one associated child: the output was identical either way. In the mixed case, with the check the loop settles on the associated child and the cleanup does remove its entries; without it the first child found is selected and nothing is removed -- harmless in these schedules, because that pass skips the toplevel commit anyway. So my earlier example is not a reason to insist on keeping the child-lookup check; I have no objection to removing it. On skipped transactions: agreed for the already-consumed transaction in this example. DecodeCommit() takes its skip branch, and ReorderBufferForget() frees the remaining tuplecids through ReorderBufferCleanupTXN(), without building the tuplecid hash on that path. I'd just keep the scope at "this pass will skip the commit": an abort record can precede start_decoding_at while the later toplevel commit still needs to be decoded, so the abort record's own skip decision cannot be used to omit the cleanup. And yes, your shorter comment is better. I would only keep one line saying that the cleanup runs before the ReorderBufferAbort() loop tears down the transactions and their associations. The helper's child-lookup comment should also distinguish missing associations from an absence of queued tuplecids on the actual toplevel. No attachment here, so cfbot keeps testing the v6 series as posted; the guard and the comment updates can go into the next version as Álvaro prefers. Best regards, Bingshuai Li
