On Tue, Sep 22, 2015 at 9:20 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-09-21 16:36:03 +0200, Andres Freund wrote:
>> Agreed. I'll update the patch.
>
> Here's updated patches against master. These include the "legacy"
> truncation support. There's no meaningful functional differences in this
> version except addressing the review comments that I agreed with, and a
> fair amount of additional polishing.
0002 looks fine.
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. I admit that I
don't see a specific way that that can go wrong, but there are so many
different old versions with slightly different multixact truncation
behaviors that it seems very hard to be sure that we're not going to
make things worse rather than better by introducing yet another
approach to the problem. 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 wish more people were paying attention to these patches. These are
critical data-corrupting bugs, the code in question is very tricky,
it's been majorly revised multiple times, and we're revising it again.
And nobody except me and Andres is looking at this, and I'm definitely
not smart enough to get this all right.
Other issues:
- 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.
Assorted minor nitpicking:
- "happend" is misspelled in the commit message for 0003
- "in contrast to before" should have a comma after it, also in that
commit message
- "how far the next members wraparound is away" -> "how far away the
next members wraparound is"
- "seing" -> "seeing"
- "Upgrade the primary," -> "Upgrade the primary;"
- "toMultiXact" -> "to MultiXact"
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
