MIME-Version: 1.0
References: <202512151349.vlq3mpfniyk3@alvherre.pgsql> <11247.1767609087@localhost>
 <11558.1767609632@localhost> <141054.1767891540@localhost>
 <CADzfLwU-OmxW3t3AoQo9=K7uq4G1yZ-txcetzW3jbcVxV_pJew@mail.gmail.com>
 <137668.1768235610@localhost> <CADzfLwUJSHKGxYw+vMUZ_Hr2YeuxO2Q5w13HKgUUN1725tjY5Q@mail.gmail.com>
In-Reply-To: <CADzfLwUJSHKGxYw+vMUZ_Hr2YeuxO2Q5w13HKgUUN1725tjY5Q@mail.gmail.com>
From: Mihail Nikalayeu <mihailnikalayeu@gmail.com>
Date: Mon, 19 Jan 2026 00:52:00 +0300
Message-ID: <CADzfLwXJ+4s1tJuG9injcxAUP3urj9D6dUAPOCaX33UeiUxrRQ@mail.gmail.com>
Subject: Re: Adding REPACK [concurrently]
To: Antonin Houska <ah@cybertec.at>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>, 
	Pg Hackers <pgsql-hackers@lists.postgresql.org>, Robert Treat <rob@xzilla.net>
Content-Type: multipart/alternative; boundary="0000000000008b66de0648b09782"
Archived-At: <https://www.postgresql.org/message-id/CADzfLwXJ%2B4s1tJuG9injcxAUP3urj9D6dUAPOCaX33UeiUxrRQ%40mail.gmail.com>
Precedence: bulk

--0000000000008b66de0648b09782
Content-Type: text/plain; charset="UTF-8"

Hello, Antonin!

Some comments for 0006:

> SnapBuildSnapshotForRepack(SnapBuild *builder)
Does it also "replays" previously processed WAL to the position that
snapshot is ready to use?
I am afraid we may see some non-yet processed parts of WAL leading to
duplicate insertion.

> first_block
What is the reason for that variable? It seems to be always the first block
of relation.
Also, what if we have a huge amount of empty space at the start. In that
case the first block will be the block of the first "filled" page. But
insert may (and will) fill empty pages before first_block - out of the
range.
So, I think we should delete it and always use 0 instead.

> DecodeMultiInsert
ItemPointerSet-related logic seems missing.

> tableScan = table_beginscan(OldHeap, SnapshotAny, 0, (ScanKey) NULL);
With SnapshotAny we are going to check the same tuple multiple times.
Better to let scan logic handle it (and change snapshots used by scan code).
See [0] for a way to reset snapshots during the scan.

> if (blkno >= range_end)
I don't think it is legal to switch a snapshot while holding the tuple.
Nothing is protecting it from being pruned\reused.
Snapshots need to be switched "between" pages. You may check how it is done
at [0].

> PopActiveSnapshot();
> InvalidateCatalogSnapshot();
I think it is a good idea to add here assert for MyProc->xmin and
MyProc->xid to be invalid. To ensure we really allow the horizon to advance.

> /* Set to request a snapshot. */
> bool snapshot_requested;
We know the end or region in advance, so it should be possible to filter
before writing changes to file.
So, it is some kind of "this is the end of region, create new file and
store everything before + create new snapshot for me".

> PopActiveSnapshot();
Sometimes without InvalidateCatalogSnapshot().

> PushActiveSnapshot(GetTransactionSnapshot());
GetLatestSnapshot() feels better here.

> * The individual builds might still be a problem, but that's a
> * separate issue.
Opening the index may create a catalog snapshot, so it needs to be
invalidated after.

> * TODO Can we somehow use the fact that the new heap is not yet
> * visible to other transaction, and thus cannot be vacuumed?
Snapshot resetting [0] may work here (without CIC, just as part of the scan
+ some code to ensure catalog snapshot is managed correctly).
Also, to correctly build a unique index - some tech from [0] is required
(building a unique index with multiple snapshots is a little bit tricky).
Or we may implement some super lightweight way - just SnapshotAny without
any visibility checks (just assume everything is ok since it copied from
another relation with the same index set).

> This approach introduces one limitation though: if the USING INDEX clause
is
> specified, an explicit sort is always used. Index scan wouldn't work
because
> it does not return the tuples sorted by CTID.

Technically we may just use keys (if they are comparable) as a way to
specify regions. Instead of number of pages to switch snapshot - number of
tuples or time.
But because we don't know the region end in advance - we have to keep all
the changes in file and filter only while applying.

> Assert(XLogRecPtrIsInvalid(shared->lsn_upto));
>  /* Initially we're expected to provide a snapshot and only that. */
>  Assert(shared->snapshot_requested &&
>    XLogRecPtrIsInvalid(shared->lsn_upto));

XLogRecPtrIsInvalid(shared->lsn_upto) assertion is duplicated here.

> range_end = repack_blocks_per_snapshot;
Should be repack_blocks_per_snapshot + ctx->first_block ? (but better to
remove the first_block at all).

> * XXX It might be worth Assert(CatalogSnapshot == NULL)
> * here, however that symbol is not external.
As said above - better assert for MyProc->xmin/xid + add
InvalidateCatalogSnapshot.

> extern Snapshot
> extern void

Some "externs" used in C files (not headers).

> ctx->block_ranges = NIL;
may be used with list_free?

>  * Remember here to which pages should applied to changes recorded in
given
>  * file.
"should apply"

>  of pages, so that VACUUM does not get block for too long
"blocked"

> there are no restriction on block
"there is" or "restrictions"

> repack_add_block_range
extra new-line after function.

> Is REPACKED (CONCURRENTLY) is being run by this backend?
REPACK and double "is"

[0]: https://commitfest.postgresql.org/patch/6401/

--0000000000008b66de0648b09782
Content-Type: text/html; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

<div dir=3D"ltr">Hello, Antonin!<div><br></div><div>Some comments for 0006:=
</div><div><br></div><div>&gt; SnapBuildSnapshotForRepack(SnapBuild *builde=
r)</div><div>Does it also &quot;replays&quot; previously processed WAL to t=
he position that snapshot is ready to use?</div><div>I am afraid we may see=
 some non-yet processed parts of WAL leading to duplicate insertion.<br></d=
iv><div><br></div><div>&gt;=C2=A0first_block </div><div>What is the reason =
for that variable? It seems to be always the first block of relation.=C2=A0=
</div><div>Also, what if we have a huge amount of empty space at the start.=
 In that case the first block will be the block of the first &quot;filled&q=
uot; page. But insert may (and will) fill empty pages before first_block - =
out of the range.</div><div>So, I think we should delete it and always use =
0 instead.</div><div><br></div><div><div>&gt;=C2=A0DecodeMultiInsert</div><=
div>ItemPointerSet-related logic seems missing.</div></div><div><br></div><=
div>&gt; tableScan =3D table_beginscan(OldHeap, SnapshotAny, 0, (ScanKey) N=
ULL);</div>With SnapshotAny we are going to check the same tuple multiple t=
imes. Better to let scan logic handle it (and change snapshots used by scan=
 code).<div><div>See [0] for a way to reset snapshots during the scan.<br><=
div><br></div><div>&gt;=C2=A0if (blkno &gt;=3D range_end)</div><div>I don&#=
39;t think it is legal to switch a snapshot while holding the tuple. Nothin=
g is protecting it from being pruned\reused.</div><div>Snapshots need to be=
 switched &quot;between&quot; pages. You may check how it is done at [0].</=
div><div><br></div><div>&gt; PopActiveSnapshot();<br>&gt; InvalidateCatalog=
Snapshot();</div><div>I think it is a good idea to add here=C2=A0assert for=
 MyProc-&gt;xmin and MyProc-&gt;xid to be invalid. To ensure we really allo=
w the horizon to advance.</div><div><br></div><div>&gt; /* Set to request a=
 snapshot. */</div>&gt; bool		snapshot_requested;</div><div>We know the end=
 or region in advance, so it should be possible to filter before writing ch=
anges to file.</div><div>So, it is some kind of &quot;this is the end of re=
gion, create new file and store everything before=C2=A0+ create new snapsho=
t for me&quot;.<br><div><br></div><div>&gt;=C2=A0PopActiveSnapshot();</div>=
<div>Sometimes=C2=A0without InvalidateCatalogSnapshot().</div><div><br></di=
v><div>&gt; PushActiveSnapshot(GetTransactionSnapshot());</div>GetLatestSna=
pshot() feels better here.</div><div><br></div><div>&gt; * The individual b=
uilds might still be a problem, but that&#39;s a<br>&gt; * separate issue.<=
/div><div>Opening the index may create a catalog snapshot, so it needs to b=
e invalidated after.</div><div><br></div><div>&gt; * TODO Can we somehow us=
e the fact that the new heap is not yet</div><div>&gt; * visible to other t=
ransaction, and thus cannot be vacuumed?</div><div>Snapshot resetting [0] m=
ay work here (without CIC, just as part of the scan + some code to ensure c=
atalog snapshot is managed correctly).</div><div>Also, to correctly build a=
 unique index - some tech from [0] is required (building a unique index wit=
h multiple snapshots is a little bit tricky).</div><div>Or we may implement=
 some super lightweight way - just SnapshotAny without any visibility check=
s (just assume everything is ok since it copied from another relation with =
the same index set).</div><div><br></div><div>&gt; This approach introduces=
 one limitation though: if the USING INDEX clause is<br>&gt; specified, an =
explicit sort is always used. Index scan wouldn&#39;t work because<br>&gt; =
it does not return the tuples sorted by CTID.</div><div><br></div><div>Tech=
nically we may just use keys (if they are comparable) as a way to specify r=
egions. Instead of number of pages to switch snapshot - number of tuples or=
 time.</div><div>But because we don&#39;t know the region end in advance - =
we have to keep=C2=A0all the changes in file and filter only while applying=
.<br><div><br></div><div>&gt; Assert(XLogRecPtrIsInvalid(shared-&gt;lsn_upt=
o));<br>&gt;=C2=A0 /* Initially we&#39;re expected to provide a snapshot an=
d only that. */<br>&gt;=C2=A0 Assert(shared-&gt;snapshot_requested &amp;&am=
p;<br>&gt;=C2=A0=C2=A0=C2=A0 XLogRecPtrIsInvalid(shared-&gt;lsn_upto));</di=
v><div><br></div><div>XLogRecPtrIsInvalid(shared-&gt;lsn_upto) assertion=C2=
=A0is duplicated here.<br></div><div><br></div><div>&gt;=C2=A0range_end =3D=
 repack_blocks_per_snapshot;</div><div>Should be repack_blocks_per_snapshot=
=C2=A0+=C2=A0ctx-&gt;first_block ? (but better to remove the first_block at=
 all).</div><div><br></div><div>&gt; * XXX It might be worth Assert(Catalog=
Snapshot =3D=3D NULL)<br>&gt; * here, however that symbol is not external.<=
br></div><div>As said above - better assert for MyProc-&gt;xmin/xid=C2=A0+ =
add InvalidateCatalogSnapshot.</div><div><br></div><div>&gt;=C2=A0extern Sn=
apshot</div><div>&gt; extern void</div><div><br></div><div>Some &quot;exter=
ns&quot; used in C files (not headers).</div><div><br></div><div>&gt; ctx-&=
gt;block_ranges =3D NIL;</div><div>may be used with list_free?<br><div><br>=
</div><div>&gt;=C2=A0 * Remember here to which pages should applied to chan=
ges recorded in given</div>&gt;=C2=A0 * file.<div>&quot;should apply&quot;<=
/div><div><br></div><div>&gt;=C2=A0=C2=A0of pages, so that VACUUM does not =
get block for too long</div><div>&quot;blocked&quot;</div><div><br></div><d=
iv>&gt;=C2=A0there are no restriction on block</div><div>&quot;there is&quo=
t; or &quot;restrictions&quot;</div><div><br></div><div>&gt;=C2=A0repack_ad=
d_block_range</div><div>extra new-line after function.</div><div><br></div>=
<div>&gt;=C2=A0Is REPACKED (CONCURRENTLY) is being run by this backend?</di=
v><div>REPACK and double &quot;is&quot;</div><div><br></div><div>[0]:=C2=A0=
<a href=3D"https://commitfest.postgresql.org/patch/6401/">https://commitfes=
t.postgresql.org/patch/6401/</a></div><div><br></div></div></div></div></di=
v>

--0000000000008b66de0648b09782--