Hi,
On 2015-09-23 15:03:05 -0300, Alvaro Herrera wrote:
> I wonder if it would be cleaner to move the setting of finishedStartup
> down to just before calling SetMultiXactIdLimit, instead of at the top
> of the function.
Done. I don't think it makes much of a difference, but there's really no
reason not to change it.
> It's a bit odd that SetMultiXactIdLimit has the "finishedStartup" test
> so low. Why bother setting all those local variables only to bail
> out?
But we do more than set local variables? We actually set some in-memory
limits (MultiXactState->multiVacLimit et al). What we can't do is to
startup the members wraparound protection because that requires
accessing the SLRUs.
Perhaps we should, independently of this patch really, rename
SetOffsetVacuumLimit() - it may be rather confusing that it actually is
about members/? The current name is correct, but also a bit
confusing. ComputeMembersVacuumLimits()?
> In MultiXactAdvanceOldest, the test for sawTruncationinCkptCycle seems
> reversed?
> if (!MultiXactState->sawTruncationInCkptCycle)
> surely we should be doing truncation if it's set?
I wanted to add a comment explaining this, but the existing job seems to
do a fair job at that:
/*
* If there has been a truncation on the master, detected by seeing a
* moving oldestMulti, without a corresponding truncation record, we
* know that the primary is still running an older version of postgres
* that doesn't yet log multixact truncations. So perform the
* truncation ourselves.
*/
I've done some additional comment smithing.
Attached is 0002 (prev 0003) including the legacy truncation support,
and 0003 removing that and bumping page magic. I'm slightly inclined to
commit them separately (to 9.5 & master) so that we have something to
backpatch from.
Greetings,
Andres Freund
