Re: REPACK enhancements

Hi Antonin,

Thanks for addressing my concerns

> Since 'reform' slot is assumed to be "virtual", a new copy should be
created
> for the insert:
> table_tuple_insert()
> -> heapam_tuple_insert()
> -> ExecFetchSlotHeapTuple(slot, materialize=true, ...)

Yes, in v02 that copy came too late. heap_freetuple(tuple) ran before
table_tuple_insert(), and the virtual slot still pointed into the freed
tuple when heapam_tuple_insert() materialized it. v03 removes the free and
asserts that src is a buffer slot with shouldFree = false, so the problem is
gone.

Here are my review for v03-0004

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

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

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

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 scan
never
reaches those blocks.

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

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 >=
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.

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 never
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 tested a 100 block table with shared_buffers = 1MB, after a cursor had
left the
scan position at block 48. With ynchronize_seqscans = off, REPACK does 6
boundaries in 0.8 s. With it on, it did 34 boundaries in 60s+. On a large
table
this would not finish in any useful time...

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

4. Assertion

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

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

Please let me know if anything is not clear.

Thanks,
Shihao