I took another look at the code coverage situation around freezing following pushing the page-level freezing patch earlier today. I spotted an issue that I'd missed up until now: certain sanity checks in heap_prepare_freeze_tuple() call TransactionIdDidCommit() more often than really seems necessary.
Theoretically this is an old issue that dates back to commit 699bf7d05c, as opposed to an issue in the page-level freezing patch. But that's not really true in a real practical sense. In practice the calls to TransactionIdDidCommit() will happen far more frequently following today's commit 1de58df4fe (since we're using OldestXmin as the cutoff that gates performing freeze_xmin/freeze_xmax processing -- not FreezeLimit). No regressions related to clog lookups by VACUUM were apparent from my performance validation of the page-level freezing work, but I suspect that the increase in TransactionIdDidCommit() calls will cause noticeable regressions with the right/wrong workload and/or configuration. The page-level freezing work is expected to reduce clog lookups in VACUUM in general, which is bound to have been a confounding factor. I see no reason why we can't just condition the XID sanity check calls to TransactionIdDidCommit() (for both the freeze_xmin and the freeze_xmax callsites) on a cheap tuple hint bit precheck not being enough. We only need an expensive call to TransactionIdDidCommit() when the precheck doesn't immediately indicate that the tuple xmin looks committed when that's what the sanity check expects to see (or that the tuple's xmax looks aborted, in the case of the callsite where that's what we expect to see). Attached patch shows what I mean. A quick run of the standard regression tests (with custom instrumentation) shows that this patch eliminates 100% of all relevant calls to TransactionIdDidCommit(), for both the freeze_xmin and the freeze_xmax callsites. -- Peter Geoghegan
v1-0001-Avoid-unnecessary-clog-lookups-when-freezing.patch
Description: Binary data