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
v2-0002-vacuum-Allow-freezing-without-a-cleanup-lock.patch
Description: Binary data
v2-0001-vacuum-Separate-heap-page-freezing-state.patch
Description: Binary data
