Re: Allow aggressive VACUUM to freeze without a cleanup lock

Hi all,

Thanks for the reply.

> Agreed. Freezing itself only needs an exclusive lock, a cleanup lock is needed
> for page defragmentation. I also agree with Melanie that the patch should
> explain this if it is not already documented elsewhere.

Yeah. Did some research from existing information.

A cleanup lock is for reorganizing a page. Pruning can remove or
change line pointers, and defragmentation can move tuple data. An
exclusive content lock does not wait for other buffer pins, so another
backend might still hold a reference to either. That is why pruning
needs a cleanup lock [1]. The WAL replay code makes the same split:
pruning needs a cleanup lock, while freezing and VACUUM's second heap
pass only need an ordinary exclusive lock [2].

[1] https://github.com/postgres/postgres/blob/master/src/backend/access/heap/README.HOT#L228-L231
[2] https://github.com/postgres/postgres/blob/master/src/include/access/heapam_xlog.h#L330-L335

> First, the buffer lock is released and reacquired between
> `lazy_scan_noprune()` and `lazy_scan_freeze()`. The page may be changed or
> already frozen during this window, so this assertion is not always valid:
>
> Second, the code may call heap_prepare_freeze_tuple() and then discard the
> freeze plans after finding an old DEAD tuple. This function may create a new
> MultiXactId, write WAL, and advance the MultiXact state. These side effects
> remain even if the plans are discarded.

Thanks for the review. I fixed both issues you found. The freeze-only
path no longer assumes anything about the page after it reacquires the
buffer lock. It also scans the whole page before it prepares freeze
plans, so a fallback to the cleanup-lock path cannot leave MultiXact or
WAL side effects behind.

> There is also a small naming issue. lazy_scan_noprune and lazy_scan_freeze are
> quite confusing now. I think lazy_scan_noprune is not correct at this moment.

The current names describe three different paths: lazy_scan_prune()
does pruning and freezing, lazy_scan_freeze() does freezing only, and
lazy_scan_noprune() does neither. The last name predates this patch,
but I agree that it is less clear now.

> The patch adds FreezeState, HeapPageFreezeParams, and
> HeapPageFreezeResult, but they overlap with PruneState,
> PruneFreezeParams, and PruneFreezeResult. These abstractions add complexity
> without much reuse.

I also agree that the parameter and result types should share a common
freeze component. PruneFreezeParams and PruneFreezeResult have extra
pruning and VM state, but their freeze-related fields overlap with the
freeze-only types. The pruning-specific state should sit around the
common part instead of having two parallel sets of structures.

v2 patches are attached.

---
Regards,
Jingtang