Hello, Antonin!
On Mon, Nov 3, 2025 at 8:56 AM Antonin Houska <ah@cybertec.at> 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.
