Re: Adding REPACK [concurrently]

Mihail Nikalayeu <mihailnikalayeu(at)gmail(dot)com> wrote:

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

All these problems are due to incorrect separation of the "preserve
visibility" part of the patch series. Will be fixed in the next version.

> Also, I think it is a good idea to add tests for index-based and
> sort-based repack.

Not sure, cluster.sql already seems to do the same.

> Also, for sort-based I think we need to also call
> repack_decode_concurrent_changes during insertion phase

I miss the point. The current coding is such that this part

if (concurrent)
{
XLogRecPtr end_of_wal;

end_of_wal = GetFlushRecPtr(NULL);
if ((end_of_wal - end_of_wal_prev) > wal_segment_size)
{
repack_decode_concurrent_changes(decoding_ctx, end_of_wal);
end_of_wal_prev = end_of_wal;
}
}

gets called regardless the value of 'tuplesort' above.

> > is_system_catalog && !concurrent
> 2 places, always true, feels strange.

ok

--
Antonin Houska
Web: https://www.cybertec-postgresql.com