freespace.c modifies buffer without any locks

From: Andres Freund <andres(at)anarazel(dot)de>
To: pgsql-hackers(at)postgresql(dot)org, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Subject: freespace.c modifies buffer without any locks
Date: 2024-10-29 00:50:00
Message-ID: aivkenhym4sublyiutfhjb2glcwlxrytoajvgiemsod763amzo@7b7qd3ftjxte
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi,

I just noticed that fsm_vacuum_page() modifies a buffer without even holding a
shared lock. That quite obviously seems like a violation of the buffer
locking protocol:

/*
* 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.
*/
if (BufferPrepareToSetHintBits(buf))
{
((FSMPage) PageGetContents(page))->fp_next_slot = 0;
BufferFinishSetHintBits(buf);
}

In the commit (15c121b3ed7) adding the current freespace code, there wasn't
even a comment remarking upon that oddity. 10 years later Tom added a
comment, in 2b1759e2675f.

I noticed this while adding a debug mode in which buffers are mprotected
PROT_NONE/PROT_READ/PROT_READ|PROT_WRITE depending on the buffer's state.

Is there any good reason to avoid a lock here? Compared to the cost of
exclusively locking buffers during RecordAndGetPageWithFreeSpace() the cost of
doing so during FreeSpaceMapVacuum*() seems small?

Somewhat relatedly, but I don't think I understand why it's a good idea to
reset fp_next_slot to 0 in fsm_vacuum_page(). At least doing so
unconditionally.

When extending a relation, it seems we'll constantly reset the search back to
the start of the range, even though we pretty much know that there's no space
earlier in the relation - otherwise we'd not have extended.

And when called from FreeSpaceMapVacuumRange() we'll reset fp_next_slot to
somewhere that wasn't actually vacuumed, afaict?

Greetings,

Andres Freund

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message David Rowley 2024-10-29 01:03:08 Re: EXPLAIN IndexOnlyScan shows disabled when enable_indexonlyscan=on
Previous Message David Rowley 2024-10-29 00:39:13 Re: Questions About TODO: Issuing NOTICEs for row count differences in EXPLAIN ANALYZE