Re: Adding REPACK [concurrently]

Hello!

On Sun, Apr 5, 2026 at 10:41 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org> wrote:
> So here's a v55 version of the base REPACK patches that I'm feeling
> comfortable calling very close to committable.

Some comments for v55.

repack.c:2725

if (!VARATT_IS_EXTERNAL(varlen))
continue;

I think it should be VARATT_IS_EXTERNAL_INDIRECT - the same as in pgrepack:244.

Also, after

natt_ext--;

I think it worth to add

Assert(natt_ext >= 0);

or Assert(natt_ext == 0); but in another place.

Or exit early with
for (int i = 0; i < desc->natts && natt_ext > 0; i++)

----------------------------
repack.c:2587

table_tuple_insert(rel, slot, GetCurrentCommandId(true),
HEAP_INSERT_NO_LOGICAL, NULL);

More idiomatic to use TABLE_INSERT_NO_LOGICAL instead.

----------------------------
repack.c:2696

ExecForceStoreHeapTuple(tup, slot, false);

AFAIU there is a memory leak here. Memory allocated above (for tuple)
is not freed in any way, because shouldFree == false.
Also, ExecClearTuple (tts_virtual_clear for virtual tuples) requires
TTS_SHOULDFREE to be set to free anything.

-------------------------
grab ShareUpdateExclusiveLock (jsut like VACUUM

typo in "just"

--------------------------

"If the identity index is not set due to replica identity being, PK"

Missing "FULL" after "being"?

-------------------------

Commit message:

"intial copy" -> "initial copy"
"backed performing REPACK" -> "backend performing REPACK"

Best regards,
Mikhail.