Re: Rework the way multixact truncations work

On Thu, Dec 03, 2015 at 07:03:21PM +0100, Andres Freund wrote:
> On 2015-12-03 04:38:45 -0500, Noah Misch wrote:
> > On Wed, Dec 02, 2015 at 04:46:26PM +0100, Andres Freund wrote:
> > > Especially if reverting and redoing includes conflicts that mainly
> > > increases the chance of accidental bugs.
> >
> > True. (That doesn't apply to these patches.)
>
> Uh, it does. You had conflicts in your process, and it's hard to verify
> that the re-applied patch is actually functionally identical given the
> volume of changes. It's much easier to see what actually changes by
> looking at iterative commits forward from the current state.

Ah, we were talking about different topics after all. I was talking about
_merge_ conflicts in a reversion commit.

> Sorry, but I really just want to see these changes as iterative patches
> ontop of 9.5/HEAD instead of this process. I won't revert the reversion
> if you push it anyway, but I think it's a rather bad idea.

I hear you. I evaluated your request and judged that the benefits you cited
did not make up for the losses I cited. Should you wish to change my mind,
your best bet is to find defects in the commits I proposed. If I introduced
juicy defects, that discovery would lend much weight to your conjectures.

> My question was more about the comment being after the "early return"
> than about the content change, should have made that clearer. Can we
> just move your comment up?

Sure, I will.

> > > > static bool
> > > > SetOffsetVacuumLimit(void)
> > > > {
> > > > - MultiXactId oldestMultiXactId;
> > > > + MultiXactId oldestMultiXactId;
> > > > MultiXactId nextMXact;
> > > > - MultiXactOffset oldestOffset = 0; /* placate compiler */
> > > > - MultiXactOffset prevOldestOffset;
> > > > + MultiXactOffset oldestOffset = 0; /* placate compiler */
> > > > MultiXactOffset nextOffset;
> > > > bool oldestOffsetKnown = false;
> > > > + MultiXactOffset prevOldestOffset;
> > > > bool prevOldestOffsetKnown;
> > > > - MultiXactOffset offsetStopLimit = 0;
> > >
> > > I don't see the benefit of the order changes here.
> >
> > I reacted the same way. Commit 4f627f8 reordered some declarations, which I
> > reverted when I refinished that commit as branch mxt3-main.
>
> But the other changes are there, and in the history anyway. As the new
> order isn't more meaningful than the current one...

Right. A revert+redo patch series can and should purge formatting changes
that did not belong in its predecessor commits. Alternate change delivery
strategies wouldn't do that.

> > > > - if (oldestOffsetKnown)
> > > > - ereport(DEBUG1,
> > > > - (errmsg("oldest MultiXactId member is at offset %u",
> > > > - oldestOffset)));
> > >
> > > That's imo a rather useful debug message.
> >
> > The branches emit that message at the same times 4f627f8^ and earlier emit it.
>
> During testing I found it rather helpful if it was emitted regularly.

I wouldn't oppose a patch making it happen more often.

> > > > LWLockRelease(MultiXactTruncationLock);
> > > >
> > > > /*
> > > > - * If we can, compute limits (and install them MultiXactState) to prevent
> > > > - * overrun of old data in the members SLRU area. We can only do so if the
> > > > - * oldest offset is known though.
> > > > + * There's no need to update anything if we don't know the oldest offset
> > > > + * or if it hasn't changed.
> > > > */
> > >
> > > Is that really a worthwhile optimization?
> >
> > I would neither remove that longstanding optimization nor add it from scratch
> > today. Branch commit 06c9979 restored it as part of a larger restoration to
> > the pre-4f627f8 structure of SetOffsetVacuumLimit().
>
> There DetermineSafeOldestOffset() did it unconditionally.

That is true; one won't be consistent with both. 06c9979 materially shortened
the final patch and eliminated some user-visible message emission changes.
Moreover, this is clearly a case of SetOffsetVacuumLimit() absorbing
DetermineSafeOldestOffset(), not vice versa.

> > > > -SlruScanDirCbFindEarliest(SlruCtl ctl, char *filename, int segpage, void *data)
> > >
> > > That really seems like an independent change, deserving its own commit +
> > > explanation.
> >
> > Indeed. I explained that change at length in
> > http://www.postgresql.org/message-id/20151108085407.GA1097830@tornado.leadboat.com,
> > including that it's alone on a branch (mxt1-disk-independent), to become its
> > own PostgreSQL commit.
>
> The comment there doesn't include the explanation...

If you visit that URL, everything from "If anything here requires careful
study, it's the small mxt1-disk-independent change, which ..." to the end of
the message is my explanation of this change. What else would you like to
know about it?

> > > > [branch commit 89a7232]
> > >
> > > I don't think that's really a good idea - this way we restart after
> > > every single page write, whereas currently we only restart after passing
> > > through all pages once. In nearly all cases we'll only ever have to
> > > retry once in total, be because such old pages aren't usually going to
> > > be reread/redirtied.
> >
> > Your improvement sounds fine, then. Would both SimpleLruTruncate() and
> > SlruDeleteSegment() benefit from it?
>
> It probably makes sense to do it in SimpleLruTruncate too - but it does
> additional checks as part of the restarts which aren't applicable for
> DeleteSegment(), which is IIRC why I didn't also change it.

Understood. There's no rule that these two functions must look as similar as
possible, so I will undo 89a7232.

Thanks,
nm