Hello, Antonin! On Mon, Nov 3, 2025 at 8:56 AM Antonin Houska <[email protected]> wrote: > I'll fix all the problems in the next version. Thanks!
A few more moments I mentioned: > switch ((vis = HeapTupleSatisfiesVacuum(tuple, OldestXmin, buf))) vis is unused, also to double braces. > LockBuffer(buf, BUFFER_LOCK_UNLOCK); > continue; > } > /* > * In the concurrent case, we have a copy of the tuple, so we > * don't worry whether the source tuple will be deleted / updated > * after we release the lock. > */ > LockBuffer(buf, BUFFER_LOCK_UNLOCK); >} I think locking and comments are a little bit confusing here. I think we may use single LockBuffer(buf, BUFFER_LOCK_UNLOCK); before `if (isdead)` as it was before. Also, I am not sure "we have a copy" is the correct point here, I think motivation is mostly the same as in heapam_index_build_range_scan. Also, I think it is a good idea to add tests for index-based and sort-based repack. Also, for sort-based I think we need to also call repack_decode_concurrent_changes during insertion phase > is_system_catalog && !concurrent 2 places, always true, feels strange. Best regards, Mikhail.
