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
# REPACK (CONCURRENTLY) with multiple snapshots: rows that land in blocks
# beyond the scan's rs_nblocks while a range boundary is being processed.
setup
{
CREATE EXTENSION injection_points;
CREATE TABLE t(i int PRIMARY KEY, j text) WITH (fillfactor = 100);
-- about 4 full pages
INSERT INTO t SELECT x, repeat('x', 200) FROM generate_series(1, 140) x;
CREATE TABLE expected(i int, j text);
}
teardown
{
DROP TABLE t;
DROP TABLE expected;
DROP EXTENSION injection_points;
}
session s1
setup
{
SET repack_snapshot_after = 2;
SELECT injection_points_set_local();
SELECT injection_points_attach('repack-concurrently-new-range', 'wait');
}
step blocks { SELECT pg_relation_size('t') / 8192 AS nblocks; }
step repack { REPACK (CONCURRENTLY) t; }
step check
{
SELECT count(*) AS missing
FROM expected e LEFT JOIN t USING (i)
WHERE t.i IS NULL;
SELECT (SELECT count(*) FROM t) AS rows_after,
(SELECT count(*) FROM expected) AS rows_expected;
}
session s2
# While REPACK waits at the first range boundary, insert rows. The table is
# full, so they go into new blocks, i.e. beyond the block count the scan saw.
step extend
{
INSERT INTO t SELECT x, repeat('y', 200) FROM generate_series(1001,
1100) x;
SELECT min(tid_block(ctid)), max(tid_block(ctid)) FROM t WHERE i > 1000;
INSERT INTO expected SELECT * FROM t;
}
step wakeup
{
SELECT injection_points_detach('repack-concurrently-new-range');
SELECT injection_points_wakeup('repack-concurrently-new-range');
}
permutation blocks repack extend wakeup check
# Blocks with no tuples at scan time are skipped without finalizing a range.
# Rows inserted there after the scan passed them are dropped by the next
# finalize_block_range() (which uses the old range end) and never copied.
setup
{
CREATE EXTENSION injection_points;
CREATE TABLE t(i int PRIMARY KEY, j text) WITH (fillfactor = 100,
autovacuum_enabled = off);
INSERT INTO t SELECT x, repeat('x', 200) FROM generate_series(1, 200) x;
CREATE TABLE expected(i int, j text);
}
teardown
{
DROP TABLE t;
DROP TABLE expected;
DROP EXTENSION injection_points;
}
session s1
setup
{
SET repack_snapshot_after = 2;
SELECT injection_points_set_local();
SELECT injection_points_attach('repack-concurrently-new-range', 'wait');
}
step repack { REPACK (CONCURRENTLY) t; }
step check
{
SELECT count(*) AS missing
FROM expected e LEFT JOIN t USING (i)
WHERE t.i IS NULL;
}
session s2
# Empty blocks 2 and 3 completely, keep 0, 1, 4, 5.
step empty
{
DELETE FROM t WHERE tid_block(ctid) IN (2, 3);
SELECT tid_block(ctid) AS blk, count(*) FROM t GROUP BY 1 ORDER BY 1;
}
step vac { VACUUM t; }
# REPACK is now at the first tuple of block 4, blocks 2-3 already passed.
step fill_gap
{
INSERT INTO t SELECT x, repeat('y', 200) FROM generate_series(1001,
1040) x;
SELECT tid_block(ctid) AS blk, count(*) FROM t WHERE i > 1000 GROUP BY
1 ORDER BY 1;
INSERT INTO expected SELECT * FROM t;
}
step wakeup
{
SELECT injection_points_detach('repack-concurrently-new-range');
SELECT injection_points_wakeup('repack-concurrently-new-range');
}
permutation empty vac repack fill_gap wakeup check
