Horton

Re: eliminate xl_heap_visible to reduce WAL (and eventually set VM on-access)

Melanie Plageman <melanieplageman@gmail.com>

View thread
From: Melanie Plageman <melanieplageman@gmail.com>
To: Kirill Reshke <reshkekirill@gmail.com>
Cc: Andres Freund <andres@anarazel.de>, Robert Haas <robertmhaas@gmail.com>, Andrey Borodin <x4mmm@yandex-team.ru>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>, Heikki Linnakangas <hlinnaka@iki.fi>
Date: 2025-11-17T23:07:27Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Remove table_scan_analyze_next_tuple unneeded parameter OldestXmin

  2. Simplify visibility check in heap_page_would_be_all_visible()

  3. Eliminate use of cached VM value in lazy_scan_prune()

  4. Combine visibilitymap_set() cases in lazy_scan_prune()

  5. Fix const qualification in prune_freeze_setup()

  6. Simplify vacuum visibility assertion

  7. Split heap_page_prune_and_freeze() into helpers

  8. Assert that cutoffs are provided if freezing will be attempted

  9. Split PruneFreezeParams initializers to one field per line

  10. Refactor heap_page_prune_and_freeze() parameters into a struct

  11. Make heap_page_is_all_visible independent of LVRelState

  12. Inline TransactionIdFollows/Precedes[OrEquals]()

  13. Add helper for freeze determination to heap_page_prune_and_freeze

  14. Bump XLOG_PAGE_MAGIC after xl_heap_prune change

  15. Correct prune WAL record opcode name in comment

  16. Add error codes when vacuum discovers VM corruption

  17. Remove unused xl_heap_prune member, reason

  18. Remove unneeded VM pin from VM replay

  19. Add assert and log message to visibilitymap_set

  20. Add error codes to some corruption log messages

Attachments

Attached v20 has general cleanup, changes to the table/index AM
callbacks detailed below, and it moves the
heap_page_prune_and_freeze() refactoring commit down the stack to
0004.

0001 - 0003 are fairly trivial cleanup patches. I think they are ready
to commit, so if I don't hear any objections in the next few days,
I'll go ahead and commit them.

On Tue, Nov 4, 2025 at 11:48 AM Melanie Plageman
<melanieplageman@gmail.com> wrote:
>
> On Wed, Oct 29, 2025 at 7:03 AM Kirill Reshke <reshkekirill@gmail.com> wrote:
> >
> > In 0010:
> >
> > I'm not terribly convenient that adding SO_ALLOW_VM_SET to TAM
> > ScanOptions is the right thing to do. Looks like VM bits are something
> > that make sense for HEAP AM for not for any TAM. So, don't we break
> > some layer of abstraction here? Would it be better for HEAP AM to set
> > some flags in heap_beginscan?
>
> I don't see another good way of doing it.
>
> The information about whether or not the relation is modified in the
> query is gathered during planning and saved in the plan. We need to
> get that information to the scan descriptor, which is all we have when
> we call heap_page_prune_opt() during the scan. The scan descriptor is
> created by the table AM implementations of scan_begin(). The table AM
> callbacks don't pass down the plan -- which makes sense; the scan
> shouldn't know about the plan. They do pass down flags, so I thought
> it made the most sense to add a flag. Note that I was able to avoid
> modifying the actual table and index AM callbacks (scan_begin() and
> ambeginscan()). I only made new wrappers that took "modifies_rel".
>
> Now, it is true that referring to the VM is somewhat of a layering
> violation. Though, other table AMs may use the information about if
> the query modifies the relation -- which is really what this flag
> represents. The ScanOptions are usually either a type or a call to
> action. Which is why I felt a bit uncomfortable calling it something
> like SO_MODIFIES_REL -- which is less of an option and more a piece of
> information. And it makes it sound like the scan modifies the rel,
> which is not the case. I wonder if there is another solution. Or maybe
> we call it SO_QUERY_MODIFIES_REL?

Attached v20 changes the ScanOption name to SO_HINT_REL_READ_ONLY and
removes the new helper functions which took modifies_rel as a
parameter. Instead it modifies the existing
table_beginscan()/index_beginscan() helpers and the relevant callbacks
they invoke to have a new flags parameter. These are additional caller
provider flags.

In master, the IndexScan structures and helpers don't use ScanOptions,
but since I'm using them for properties of the base relation, I think
it is fine. I'm not sure if I should name the parameter base_rel_flags
instead of flags for the index-related callbacks and helpers or if
leaving it more generic is better, though.

- Melanie