On 25/11/2025 22:46, Andres Freund wrote:
On 2025-11-25 15:02:02 -0500, Melanie Plageman wrote:
On Tue, Nov 25, 2025 at 11:54 AM Andres Freund <[email protected]> wrote:
--- a/src/backend/storage/freespace/freespace.c
+++ b/src/backend/storage/freespace/freespace.c
@@ -904,14 +904,22 @@ fsm_vacuum_page(Relation rel, FSMAddress addr,
max_avail = fsm_get_max_avail(page);
/*
- * Reset the next slot pointer. This encourages the use of low-numbered
- * pages, increasing the chances that a later vacuum can truncate the
- * relation. We don't bother with a lock here, nor with marking the page
- * dirty if it wasn't already, since this is just a hint.
+ * Try to reset the next slot pointer. This encourages the use of
+ * low-numbered pages, increasing the chances that a later vacuum can
+ * truncate the relation. We don't bother with a lock here, nor with
+ * marking the page dirty if it wasn't already, since this is just a hint.
+ *
+ * To be allowed to update the page without an exclusive lock, we have to
+ * use the hint bit infrastructure.
*/
What the heck? This didn't even take a share lock before...
It is insane. I do not understand how anybody thought this was ok. I think I
probably should split this out into a separate commit, since it's so insane.
Aren't you going to backport having it take a lock? Then it can be
separate (e.g. have it take a share lock in the "fix" commit and then
this commit bumps it to share-exclusive).
I wasn't thinking we should backport that. It's been this way for ages,
without having caused known issues....
I wrote that originally :-). The FSM code always treats the FSM page
contents as potentially corrupted garbage. FSM page updates are not
WAL-logged, for starters. And as the comment says, the next slot pointer
is just a hint for where within the page to start looking for free space.
Page checksums were added later, and now we know about the problems of
modifying a page a write() is in progress, on some filesystems with
filesystem-level checksums. So it's a good idea to tighten it up, but it
was fine back then.
diff --git a/src/backend/storage/freespace/fsmpage.c
b/src/backend/storage/freesp>
/*
* Update the next-target pointer. Note that we do this even if we're only
* holding a shared lock, on the grounds that it's better to use a shared
* lock and get a garbled next pointer every now and then, than take the
* concurrency hit of an exclusive lock.
We appear to avoid the garbling now?
I don't think so. Two backends concurrently can do fsm_search_avail() and
one backend might set a hint to a page that is already used up by the other
one. At least I think so?
Maybe I don't know what it meant by garbled, but I thought it was
talking about two backends each trying to set fp_next_slot. If they
now have to have a share-exclusive lock and they can't both have a
share-exclusive lock at the same time, then it seems like that
wouldn't be a problem. It sounds like you may be talking about a
backend taking up the freespace of a page that is referred to by the
fp_next_slot?
Yes, a version of the latter. The value that fp_next_slot will be set to can
be outdated by the time we actually set it, unless we do all of
fsm_search_avail() under some form of exclusive lock - clearly not something
desirable.
I'm pretty sure the "garbled" in the comment means the former, not the
latter. I.e. it means that the pointer itself might become garbage.
Would be good to update the comment if that's no longer possible.
But speaking of that: why do we not allow two processes to concurrently
set hint bits on a page anymore?
- Heikki
