Hello!
On Sun, Apr 5, 2026 at 10:41 PM Alvaro Herrera <alvherre@alvh.no-ip.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.
