Subject: Re: Why mark empty pages all visible?

From: Peter Geoghegan <[email-masked]>
Date: Mon, 27 Mar 2023 21:51:09 -0700

On Mon, Mar 27, 2023 at 9:32 PM Andres Freund <[email-masked]> wrote:
> On 2023-03-27 20:12:11 -0700, Peter Geoghegan wrote:
> > On Mon, Mar 27, 2023 at 6:48 PM Andres Freund <[email-masked]> wrote:
> > > It seems odd that we enter the page into the VM at this point. That means that
> > > use of that page will now require a bit more work (including
> > > RelationGetBufferForTuple() pinning it).
> >
> > I think that it's fairly obvious that it's *not* odd at all. If it
> > didn't do this then the pages would have to be scanned by VACUUM.
>
> Yes - just like in the case of new pages.

I'm not saying that the status quo is free of contradictions. Only
that there seem to be contradictions in what you're saying now.

> > You haven't said anything about the leading cause of marking empty
> > pages all-frozen, by far: lazy_vacuum_heap_page(). Should it also stop
> > marking empty pages all-frozen?
>
> It's not obvious that it should - but it's not as clear a case as the
> ConditionalLockBufferForCleanup() -> lazy_scan_new_or_empty() one. In the
> latter, we know
> a) that we don't have to do any work to be able to advance the horizon
> b) we know that somebody else has the page pinned
>
> What's the point in marking it all-visible at that point? In quite likely be
> from RelationGetBufferForTuple() having extended the relation and then briefly
> needed to release the lock (to acquire the lock on otherBuffer or in
> GetVisibilityMapPins()).

I think that there is significant value in avoiding special cases, on
general principle. If we stopped doing this in
lazy_scan_new_or_empty() we'd be inventing a new thing that cleanup
locks are supposed to protect against. Maybe something like that would
make sense, but if so then make that argument, and make it explicitly
represented in the code.

> I don't follow. In the ConditionalLockBufferForCleanup() ->
> lazy_scan_new_or_empty() case we are dealing with an new or empty
> page. Whereas lazy_vacuum_heap_page() deals with a page that definitely has
> dead tuples on it.  How are those two cases comparable?

It doesn't have dead tuples anymore, though.

ISTM that there is an issue here with the definition of an empty page.
You're concerned about PageIsEmpty() pages. Which actually aren't
quite the same thing as an empty page left behind by
lazy_vacuum_heap_page(). It's just that this distinction isn't quite
acknowledged anywhere, and probably didn't exist at all at some point.
Maybe that should be addressed.

> > > It seems pretty clear that we shouldn't enter a currently-in-use page into the
> > > VM or freespacemap. All that's going to do is to "disturb" the backend trying
> > > to use that page (by directing other backends to it) and to make its job more
> > > expensive.
> >
> > I don't think that it's clear. What about the case where there is only
> > one tuple, on a page that we cannot cleanup lock? Where do you draw
> > the line?
>
> I don't see how that's comparable? For one, we might need to clean up that
> tuple for vacuum to be able to advance the horizon. And as far as the
> non-cleanup lock path goes, it actually can perform work there. And it doesn't
> even need to acquire an exclusive lock.

So we should put space in the FSM if it has one tuple, but not if it
has zero tuples? Though not if it has zero tuples following processing
by lazy_vacuum_heap_page()?

-- 
Peter Geoghegan

---

From: Andres Freund <[email-masked]>
Date: Tue, 28 Mar 2023 12:01:38 -0700

Hi,

> make sense, but if so then make that argument, and make it explicitly

I will probably make that argument - so far I was just trying to understand
the intent of the current code. There aren't really comments explaining why we
want to mark currently-pinned empty/new pages all-visible and enter them into
the FSM.

Historically we did *not* enter currently pinned empty/new pages into the FSM
/ VM. Afaics that's new as of 44fa84881fff.

The reason I'm looking at this is that there's a lot of complexity at the
bottom of RelationGetBufferForTuple(), related to needing to release the lock
on the newly extended page and then needing to recheck whether there still is
free space on the page. And that it's not complicated enough
(c.f. INSERT_FROZEN doing visibilitymap_pin() with the page locked).

As far as I can tell, if we went back to not entering new/empty pages into
either VM or FSM, we could rely on the page not getting filled while just
pinning, not locking it.

> > page. Whereas lazy_vacuum_heap_page() deals with a page that definitely
> > has dead tuples on it.  How are those two cases comparable?
> 
> You're concerned about PageIsEmpty() pages.

And not just any PageIsEmpty() page, ones that are currently pinned.

I also do wonder whether the different behaviour of PageIsEmpty() and
PageIsNew() pages makes sense. The justification for not marking PageIsNew()
pages as all-visible holds just as true for empty pages. There's just as much
free space there.

Greetings,

Andres Freund

---

From: Peter Geoghegan <[email-masked]>
Date: Tue, 28 Mar 2023 13:05:19 -0700

> want to mark currently-pinned empty/new pages all-visible and enter them into

I don't think that not being able to immediately get a cleanup lock on
a page signifies much of anything. I never have.

> / VM. Afaics that's new as of 44fa84881fff.

Of course that's true, but I don't know why that history is
particularly important. Either way, holding a pin was never supposed
to work as an interlock against a page being concurrently set
all-visible, or having its space recorded in the FSM. It's true that
my work on VACUUM has shaken out a couple of bugs where we
accidentally relied on that being true. But those were all due to the
change in lazy_vacuum_heap_page() made in Postgres 14 -- not the
addition of lazy_scan_new_or_empty() in Postgres 15.

I actually think that I might agree with the substance of much of what
you're saying, but at the same time I don't think that you're defining
the problem in a way that's particularly helpful. I gather that you
*don't* want to do anything about the lazy_vacuum_heap_page behavior
with setting empty pages all-visible (just the lazy_scan_new_or_empty
behavior). So clearly this isn't really about marking empty pages
all-visible, with or without a cleanup lock. It's actually about
something rather more specific: the interactions with
RelationGetBufferForTuple.

I actually agree that VACUUM is way too unconcerned about interfering
with concurrent activity in terms of how it manages free space in the
FSM. But this seems like just about the least important example of
that (outside the context of your RelationGetBufferForTuple work). The
really important case (that VACUUM gets wrong) all involve recently
dead tuples. But I don't think that you want to talk about that right
now. You want to talk about RelationGetBufferForTuple.

> either VM or FSM, we could rely on the page not getting filled while just

What you're essentially arguing for is inventing a new rule that makes
the early lifetime of a page (what we currently call a PageIsEmpty()
page, and new pages) special, to avoid interference from VACUUM. I
have made similar arguments myself on quite a few occasions, so I'm
actually sympathetic. I just think that you should own it. And no, I'm
not just reflexively defending my work in 44fa84881fff; I actually
think that framing the problem as a case of restoring a previous
behavior is confusing and ahistorical. If there was a useful behavior
that was lost, then it was quite an accidental behavior all along. The
difference matters because now you have to reconcile what you're
saying with the lazy_vacuum_heap_page no-cleanup-lock behavior added
in 14.

I think that you must be arguing for making the early lifetime of a
heap page special to VACUUM, since AFAICT you want to change VACUUM's
behavior with strictly PageIsEmpty/PageIsNew pages only -- and *not*
with pages that have one remaining LP_UNUSED item, but are otherwise
empty (which could be set all-visible/all-frozen in either the first
or second heap pass, even if we disabled the lazy_scan_new_or_empty()
behavior you're complaining about). You seem to want to distinguish
between very new pages (that also happen to be empty), and old pages
that happen to be empty. Right?

> pages as all-visible holds just as true for empty pages. There's just as much

What you say here makes a lot of sense to me. I'm just not sure what
I'd prefer to do about it.

-- 
Peter Geoghegan

---

From: Andres Freund <[email-masked]>
Date: Tue, 28 Mar 2023 15:29:50 -0700

Hi,

> a page signifies much of anything. I never have.

Why is that? It's as clear a signal of concurrent activity on the buffer
you're going to get.

> particularly important.

It's interesting to understand *why* we are doing what we are. I think it'd
make sense to propose changing how things work around this, but I just don't
feel like I have a good enough grasp for why we do some of the things we
do. And given there's not a lot of comments around it and some of the comments
that do exist are inconsistent with themselves, looking at the history seems
like the next best thing?

> the problem in a way that's particularly helpful.

Likely because the goals of the existing code aren't clear to me. So I don't
feel like I have a firm grasp...

> I gather that you *don't* want to do anything about the
> lazy_vacuum_heap_page behavior with setting empty pages all-visible (just
> the lazy_scan_new_or_empty behavior).

Not in the short-medium term, at least. In the long term I do suspect we might
want to do something about it.  We have a *crapton* of contention in the FSM
and caused by the FSM in bulk workloads. With my relation extension patch
disabling the FSM nearly doubles concurrent load speed.

At the same time, the fact that we might loose knowledge about all the
existing free space in case of promotion or crash and never rediscover that
space (because the pages are frozen), seems decidedly not great.

I don't know what the path forward is, but it seems somewhat clear that we
ought to do something. I suspect having a not crash-safe FSM isn't really
acceptable anymore - it probably is fine to not persist a *reduction* in free
space, but we can't permanently loose track of free space, no matter how many
crashes.

I know that you strongly dislike the way the FSM works, although I forgot some
of the details.

One thing I am fairly certain about is that using the FSM to tell other
backends about newly bulk extended pages is not a good solution, even though
we're stuck with it for the moment.

> now. You want to talk about RelationGetBufferForTuple.

That's indeed the background. By now I'd also like to add a few comments
explaining why we do what we currently do, because I don't find all of it
obvious.

> saying with the lazy_vacuum_heap_page no-cleanup-lock behavior added

I really don't have a position to own yet, not on firm enough ground.

> that happen to be empty. Right?

I think that might be worthwhile, yes. The retry code in
RelationGetBufferForTuple() is quite hairy and almost impossible to test. If
we can avoid the complexity, at a fairly bound cost (vacuum needing to
re-visit new/empty pages if they're currently pinned), it'd imo be more that
worth the price.

But perhaps the better path forward is to just bite the bullet and introduce a
shared memory table of open files, that contains "content size" and "physical
size" for each relation.  We've had a lot of things over the years that'd have
benefitted from that.

To address the RelationGetBufferForTuple() case, vacuum would simply not scan
beyond the "content size", so it'd never encounter the page that
RelationGetBufferForTuple() is currently dealing with.  And we'd not need to
add bulk extended pages into the FSM.

This would also, I think, be the basis for teaching vacuum to truncate
relations without acquiring an AEL - which IME causes a lot of operational
issues.

It'd not do anything about loosing track of free space though :/.

> I'd prefer to do about it.

You and me both...

I wonder what it'd take to make the FSM "more crashsafe". Leaving aside the
cases of "just extended" new/empty pages: We already call
XLogRecordPageWithFreeSpace() for HEAP2_VACUUM, HEAP2_VISIBLE,
XLOG_HEAP2_PRUNE as well as insert/multi_insert/update. However, we don't set
the LSN of FSM pages. Which means we'll potentially flush dirty FSM buffers to
disk, before the corresponding WAL records make it to disk.

ISTM that even if we just used the LSN of the last WAL record for
RecordPageWithFreeSpace()/LogRecordPageWithFreeSpace(), and perhaps also
called LogRecordPageWithFreeSpace() during XLOG_HEAP2_FREEZE replay, we'd
*drastically* shrink the chance of loosing track of free space. Obviously not
free, but ISTM that it can't add a lot of overhead.

I think we can loose the contents of individual leaf FSM pages, e.g. due to
checksum failures - but perhaps we could address that on access, e.g. by
removing the frozen bit for the corresponding heap pages, which'd lead us to
eventually rediscover the free space?

That'd still leve us with upper level corruption, but I guess we could just
recompute those in some circumstances?

Hm - it'd sure be nice if pg_buffercache would show the LSN of valid pages...

Greetings,

Andres Freund