From: Antonin Houska <ah@cybertec.at>
To: shihao zhong <zhong950419@gmail.com>
cc: alvherre@kurilemu.de, pgsql-hackers@lists.postgresql.org
Subject: Re: REPACK enhancements
In-reply-to: 
 <CAGRkXqRH2aEVAibX=nhgb1Z5JZj0b2mG4VrmwZY3BkMQbrs8nQ@mail.gmail.com>
References: <109367.1781614382@localhost> <108776.1784105248@localhost>
 <CAGRkXqQoWEzT9C4p3CO-oKTXLjeyDG-RuoKHcQ5AUi8AAEhgcw@mail.gmail.com>
 <224072.1789577949@localhost>
 <CAGRkXqRH2aEVAibX=nhgb1Z5JZj0b2mG4VrmwZY3BkMQbrs8nQ@mail.gmail.com>
Comments: In-reply-to shihao zhong <zhong950419@gmail.com>
   message dated "Wed, 16 Sep 2026 22:03:49 -0400."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <38164.1789662971.1@localhost>
Content-Transfer-Encoding: quoted-printable
Date: Thu, 17 Sep 2026 18:36:11 +0200
Message-ID: <38165.1789662971@localhost>
Archived-At: 
 <https://www.postgresql.org/message-id/38165.1789662971%40localhost>
Precedence: bulk

shihao zhong <zhong950419@gmail.com> wrote:

> Here are my review for v03-0004
> =

> 1. =

> In copy_table_data(), the old "else use_sort =3D false" belonged to the
> "OldIndex !=3D NULL && btree" test. After the change, the same "else" be=
longs
> to "if (!concurrent)", and the inner test has no else.

> On my machine VACUUM FULL pg_am segfaults in
> tuplesort_begin_cluster() with indexRel =3D NULL. =

> =

> This also makes 14 tests fail, with "bool use_sort =3D false;" all tests=
 pass.

Yes, it looks like use_sort is can be left uninitialized in some cases.

> 2. =

> Looks like some rows are lost when the table grows/gap fill.
> =

> heapScan->rs_nblocks is fixed when the scan starts. At a range boundary,=
 =

> changes to blocks at or beyond rs_nblocks are not in [range_start, range=
_end), =

> so they are skipped. The next snapshot can see those tuples, but the sca=
n never
> reaches those blocks.
> =

> The attached extend.spec has 5 blocks. It uses
> repack_snapshot_after =3D 2, pauses at the first boundary, and inserts 1=
00
> rows. 70 of them go to blocks 5 to 7 and are missing after REPACK.

Interestiong. I think we need to get the new rows from new blocks each tim=
e we
process the concurrent changes.

> The boundary is only checked when the scan returns a tuple. If blocks 2 =
and
> 3 are empty (DELETE plus VACUUM, which is a common reason to run REPACK)=
,
> the scan passes them silently. The boundary fires at block 4, and
> finalize_block_range() replays with the old range_end (2). Rows inserted
> into blocks 2 and 3 after the scan passed them are skipped, and the scan
> does not go back. gap.spec loses 40 of 40 inserted rows.
> =

> One quick fix made the rows come back and kept the suites
> green. It passes "cur" instead of the old end to
> repack_process_concurrent_changes(), and it treats any block >=3D
> rs_nblocks as in range. It may be cleaner to drive the ranges by block
> number, for example with heap_setscanlimits(), than by the first tuple
> returned.

ok, I think I understand the problem.

> 3. Synchronized seqscan
> =

> table_beginscan() allows syncscan, so on a table larger than
> shared_buffers / 4 the scan can start in the middle. range_start is neve=
r
> updated after that. Once the scan wraps to block 0, "blkno < range_start=
"
> is true for every tuple, and every tuple goes through
> finalize_block_range(). =


I had the wraparound in mind when writing the patch, but it's possible tha=
t I
missed something. I don't understand how "blkno < range_start" is true for
every tuple, since finalize_block_range() should update the range
boundaries. I need to spend more time on it.

> I tested a 100 block table with shared_buffers =3D 1MB, after a cursor h=
ad left the =

> scan position at block 48. With ynchronize_seqscans =3D off,  REPACK doe=
s 6 =

> boundaries in 0.8 s. With it on, it did 34 boundaries in 60s+. On a larg=
e table
> this would not finish in any useful time... =

> =

> I think the simplest fix is table_beginscan_strat(..., allow_sync =3D
> false) in the CONCURRENTLY case. Then the wraparound code can go away.

I'd prefer handling the wraparound correctly.

> 4. Assertion
> =

> The new Assert(!IsolationUsesXactSnapshot()) is not guarded by the
> transaction block check:
>   SET default_transaction_isolation =3D 'repeatable read';
>   REPACK (CONCURRENTLY) t;
> TRAP: failed Assert("!IsolationUsesXactSnapshot()"), File: "repack.c"
> This needs an error, or the new transaction should force READ COMMITTED.

Probably ERROR.

> Would you be ok if I post fixes for some of these as patches on top of y=
our series?
> I know parts of the design are still open, but I think code is easier to=
 discuss
> than a description.

Definitely. (Please prepend the diffs with "nocfbot" so that cfbot ignores=
 them.)

Thanks!

-- =

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