The comment on top of TrimMultiXact states that "no locks are needed
here", but then goes on to grab a few locks. I think we should remove
the comment, or rephrase it to state that we still grab them for
consistency or whatever; or perhaps even remove the lock acquisitions.
(I think the comment is still true: by the time TrimMultiXact runs,
we're out of recovery but not yet running, so it's not possible for
anyone to try to do anything multixact-related.)
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.
It's a bit odd that SetMultiXactIdLimit has the "finishedStartup" test
so low. Why bother setting all those local variables only to bail out?
I think it would make more sense to just do it at the top. The only
thing you lose AFAICS is that elog(DEBUG1) message -- is that worth it?
Also, the fact that finishedStartup itself is read without a lock at
least merits a comment.
In MultiXactAdvanceOldest, the test for sawTruncationinCkptCycle seems
reversed?
if (!MultiXactState->sawTruncationInCkptCycle)
surely we should be doing truncation if it's set?
Honestly, I wonder whether this message
ereport(LOG,
(errmsg("performing legacy multixact
truncation"),
errdetail("Legacy truncations are
sometimes performed when replaying WAL from an older primary."),
errhint("Upgrade the primary, it is
susceptible to data corruption.")));
shouldn't rather be a PANIC. (The main reason not to, I think, is that
once you see this, there is no way to put the standby in a working state
without recloning).
I think the prevOldestOffsetKnown test in line 2667 ("if we failed to
get ...") is better expressed as an else-if of the previous "if" block.
I think the two "there are NO MultiXacts" cases in TruncateMultiXact
would benefit in readability from adding braces around the lone
statement (and moving the comment to the line prior).
If the find_multixact_start(oldestMulti) call in TruncateMultiXact
fails, what recourse does the user have? I wonder if the elog() should
be a FATAL instead of just LOG. It's not like it would work on a
subsequent run, is it?
--
Álvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
