Re: Rework the way multixact truncations work - Mailing list pgsql-hackers

On Tue, Sep 22, 2015 at 1:57 PM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-09-22 13:38:58 -0400, Robert Haas wrote:
>> Regarding 0003, I'm still very much not convinced that it's a good
>> idea to apply this to 9.3 and 9.4.  This patch changes the way we do
>> truncation in those older releases; instead of happening at a
>> restartpoint, it happens when oldestMultiXid advances.
>
> The primary reason for doing that is that doing it at restartpoints is
> simply *wrong*. Restartpoints aren't scheduled in sync with replay -
> which means that a restartpoint can (will actually) happen long long
> after the checkpoint from the primary has replayed.  Which means that by
> the time the restartpoint is performed it's actually not unlikely that
> we've already filled all slru segments. Which is bad if we then fail
> over/start up.

1. It would be possible to write a patch that included ONLY the
changes needed to make that happen, and did nothing else.  It would be
largely a subset of this.  If we want to change 9.3 and 9.4, I
recommend we do that first, and then come back to the rest of this.

2. I agree that what we're doing right now is wrong.  And I agree that
this fixes a real problem. But it seems to me to be quite possible,
even likely, that it will create other problems.

For example, suppose that there are files in the data directory that
precede oldestMultiXact. In the current approach, we'll remove those
because they're not in the range we expect to be used.  But in this
approach we no longer remove everything we think shouldn't be there.
We remove exactly the stuff we think should go away.  As a general
principle, that's clearly superior.  But in the back-branches, it
creates a risk: a leftover old file that doesn't get removed the first
time through - for whatever reason - becomes a time bomb that will
explode on the next wraparound.  I don't know that that will happen.
But I sure as heck don't know that won't happen with any combination
of the variously broken 9.3.X releases we've put out there.  Even if
you can prove that particular risk never materializes to your
satisfaction and mine, I will bet you a beer that there are other
possible hazards neither of us is foreseeing right now.

>> I realize that you disagree and will probably commit this to those
>> branches anyway. But I want it to be clear that I don't endorse that.
>
> I don't plan to commit/backpatch this over your objection.

I'm not in a position to demand that you take my advice, but I'm
telling you what I think as honestly as I know how.

To be clear, I am fully in favor of making these changes (without the
legacy truncation stuff) in 9.5 and master, bumping WAL page magic so
that we invalidate any 9.5 alpha standys.  I think it's a far more
solid approach than what we've got right now, and it clearly
eliminates a host of dangers.  In fact, I think it would be a pretty
stupid idea not to make these changes in those branches.  It would be
doubling down on a design we know can never be made robust.

But I do not have confidence that we can change 9.4 and especially 9.3
without knock-on consequences.  You may have that confidence.  I most
definitely do not.  My previous two rounds in the boxing ring with
this problem convinced me that (1) it's incredibly easy to break
things with well-intentioned changes in this area, (2) it's
practically impossible to foresee everything that might go wrong with
some screwy combination of versions, and (3) early 9.3.X releases are
in much worse shape than early 9.4.X releases, to the point where
guessing what any given variable is going to contain on 9.3.X is
essentially throwing darts at the wall.  That's an awfully challenging
environment in which to write a bullet-proof patch.

>> - If SlruDeleteSegment fails in unlink(), shouldn't we at the very
>> least log a message?  If that file is still there when we loop back
>> around, it's going to cause a failure, I think.
>
> The existing unlink() call doesn't, that's the only reason I didn't add
> a message there. I'm fine with adding a (LOG or WARNING?) message.

Cool.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company