MIME-Version: 1.0
References: <202604051852.qfdnfvemfkd2@alvherre.pgsql>
 <202604052035.il2hp2wze6pz@alvherre.pgsql>
In-Reply-To: <202604052035.il2hp2wze6pz@alvherre.pgsql>
From: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Date: Mon, 6 Apr 2026 12:14:57 +0200
Message-ID: 
 <CADzfLwUB=iafkv2dR-9oFgjAANRZE2icUJQyt-zLpcs_T_ug8w@mail.gmail.com>
Subject: Re: Adding REPACK [concurrently]
To: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Antonin Houska <ah@cybertec.at>,
 Srinath Reddy Sadipiralla <srinath2133@gmail.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Matthias van de Meent <boekewurm+postgres@gmail.com>,
	Pg Hackers <pgsql-hackers@lists.postgresql.org>,
 Robert Treat <rob@xzilla.net>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CADzfLwUB%3Diafkv2dR-9oFgjAANRZE2icUJQyt-zLpcs_T_ug8w%40mail.gmail.com>
Precedence: bulk

Hello!

On Sun, Apr 5, 2026 at 10:41=E2=80=AFPM 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 >=3D 0);

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

Or exit early with
     for (int i =3D 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 =3D=3D 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.