On Tue, May 24, 2016 at 03:01:13PM -0400, Tom Lane wrote:
> Also, I notice another problem in vac_truncate_clog() now that I'm looking
> at it: it's expecting that the pg_database datfrozenxid and datminmxid
> values will hold still while it's looking at them. Since
> vac_update_datfrozenxid updates those values in-place, there's a race
> condition against VACUUMs happening in other databases. We should fetch
> those values into local variables before doing the various tests inside
> the scan loop.
Commit 2d2e40e fixed the above. There's another problem just like it, one
layer lower. vac_update_datfrozenxid() has:
if (TransactionIdPrecedes(classForm->relfrozenxid, newFrozenXid))
newFrozenXid = classForm->relfrozenxid;
classForm points to buffer memory, and vac_update_relstats() inplace-updates
the buffer. Like vac_truncate_clog(), we don't mind using an old value, but
those two lines must use the same value. The attached test case shows this
bug making datfrozenxid move ahead of relfrozenxid. The attached patch fixes
it. (I noticed this while finishing up patches for the heap_inplace_update
writer race in https://postgr.es/m/20231102030915.d3.nmisch@google.com.)
I audited other read-only use of inplace-updated fields. Others look safe,
because they hold rel locks that exclude VACUUM, or they make only
non-critical decisions. Still, let's change some to the load-once style, to
improve the chance of future copy/paste finding the safe style. I'm attaching
a patch for that, too. I didn't add "volatile", because I couldn't think of
how we'd care if the load moved earlier.