Re: Removing more vacuumlazy.c special cases, relfrozenxid optimizations

On Fri, Feb 18, 2022 at 4:10 PM Peter Geoghegan <pg@bowt.ie> wrote:
> It does not change any other behavior. It's totally mechanical.
>
> 0001 is tricky in the sense that there are a lot of fine details, and
> if you get any one of them wrong the result might be a subtle bug. For
> example, the heap_tuple_needs_freeze() code path is only used when we
> cannot get a cleanup lock, which is rare -- and some of the branches
> within the function are relatively rare themselves. The obvious
> concern is: What if some detail of how we track the new relfrozenxid
> value (and new relminmxid value) in this seldom-hit codepath is just
> wrong, in whatever way we didn't think of?

Right. I think we have no choice but to accept such risks if we want
to make any progress here, and every patch carries them to some
degree. I hope that someone else will review this patch in more depth
than I have just now, but what I notice reading through it is that
some of the comments seem pretty opaque. For instance:

+ * Also maintains *NewRelfrozenxid and *NewRelminmxid, which are the current
+ * target relfrozenxid and relminmxid for the relation.  Assumption is that

"maintains" is fuzzy. I think you should be saying something much more
explicit, and the thing you are saying should make it clear that these
arguments are input-output arguments: i.e. the caller must set them
correctly before calling this function, and they will be updated by
the function. I don't think you have to spell all of that out in every
place where this comes up in the patch, but it needs to be clear from
what you do say. For example, I would be happier with a comment that
said something like "Every call to this function will either set
HEAP_XMIN_FROZEN in the xl_heap_freeze_tuple struct passed as an
argument, or else reduce *NewRelfrozenxid to the xmin of the tuple if
it is currently newer than that. Thus, after a series of calls to this
function, *NewRelfrozenxid represents a lower bound on unfrozen xmin
values in the tuples examined. Before calling this function, caller
should initialize *NewRelfrozenxid to <something>."

+                        * Changing nothing, so might have to ratchet
back NewRelminmxid,
+                        * NewRelfrozenxid, or both together

This comment I like.

+                        * New multixact might have remaining XID older than
+                        * NewRelfrozenxid

This one's good, too.

+ * Also maintains *NewRelfrozenxid and *NewRelminmxid, which are the current
+ * target relfrozenxid and relminmxid for the relation.  Assumption is that
+ * caller will never freeze any of the XIDs from the tuple, even when we say
+ * that they should.  If caller opts to go with our recommendation to freeze,
+ * then it must account for the fact that it shouldn't trust how we've set
+ * NewRelfrozenxid/NewRelminmxid.  (In practice aggressive VACUUMs always take
+ * our recommendation because they must, and non-aggressive VACUUMs always opt
+ * to not freeze, preferring to ratchet back NewRelfrozenxid instead).

I don't understand this one.

+        * (Actually, we maintain NewRelminmxid differently here, because we
+        * assume that XIDs that should be frozen according to cutoff_xid won't
+        * be, whereas heap_prepare_freeze_tuple makes the opposite assumption.)

This one either.

I haven't really grokked exactly what is happening in
heap_tuple_needs_freeze yet, and may not have time to study it further
in the near future. Not saying it's wrong, although improving the
comments above would likely help me out.

> > If there's a way you can make the precise contents of 0002 and 0003
> > more clear, I would like that, too.
>
> The really big one is 0002 -- even 0003 (the FSM PageIsAllVisible()
> thing) wasn't on the table before now. 0002 is the patch that changes
> the basic criteria for freezing, making it block-based rather than
> based on the FreezeLimit cutoff (barring edge cases that are important
> for correctness, but shouldn't noticeably affect freezing overhead).
>
> The single biggest practical improvement from 0002 is that it
> eliminates what I've called the freeze cliff, which is where many old
> tuples (much older than FreezeLimit/vacuum_freeze_min_age) must be
> frozen all at once, in a balloon payment during an eventual aggressive
> VACUUM. Although it's easy to see that that could be useful, it is
> harder to justify (much harder) than anything else. Because we're
> freezing more eagerly overall, we're also bound to do more freezing
> without benefit in certain cases. Although I think that this can be
> justified as the cost of doing business, that's a hard argument to
> make.

You've used the term "freezing cliff" repeatedly in earlier emails,
and this is the first time I've been able to understand what you
meant. I'm glad I do, now.

But can you describe the algorithm that 0002 uses to accomplish this
improvement? Like "if it sees that the page meets criteria X, then it
freezes all tuples on the page, else if it sees that that individual
tuples on the page meet criteria Y, then it freezes just those." And
like explain what of that is same/different vs. now.

Thanks,

-- 
Robert Haas
EDB: http://www.enterprisedb.com