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

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