Re: REPACK enhancements

shihao zhong <zhong950419(at)gmail(dot)com> wrote:

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

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

Interestiong. I think we need to get the new rows from new blocks each time 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 >=
> 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 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 had the wraparound in mind when writing the patch, but it's possible that 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 = 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.

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 = '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 your 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