REPACK enhancements

Antonin Houska <ah@cybertec.at> wrote:

This patch set is for the next development cycle. It tries to relax some
limitations of the REPACK (CONCURRENTLY) command.

This is the patch set rebased. (It also fixes race conditions in
repack_snapshots.spec that I haven't encountered before).

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

Hi Antonin,

Alvaro suggested I review this thread. Starting with the preparatory
refactors 0001-0003, which stand on their own. They apply on ca326e903df
(needs a rebase on current master), build with --cassert, and the
injection_points/isolation and test_decoding suites pass.

0001: in heap_insert_for_repack(), the reform slot borrows the source
tuple's by-ref data (ExecForceStoreHeapTuple with shouldFree=false), but
'tuple' is freed before the insert reads from it -- a use-after-free if
ExecFetchSlotHeapTuple(src) returns shouldFree=true. Latent today (the copy
paths pass buffer/heaptuple slots), but the "nothing should depend on it
now" comment is wrong. Suggest freeing after the insert; that also drops the
slot-type assumption.

0002: pure code motion, LGTM.

0003: factors the per-destination fields of ChangeContext into a reusable
RepackDest, so 0004 can target the auxiliary relation as well as the new
heap.

Also, could you register this in the open commitfest so cfbot tracks it?

Testing 0004 now, will post new finding in following emails

Thanks,
Shihao

shihao zhong <zhong950419@gmail.com> wrote:

0001: in heap_insert_for_repack(), the reform slot borrows the source
tuple's by-ref data (ExecForceStoreHeapTuple with shouldFree=false), but
'tuple' is freed before the insert reads from it -- a use-after-free if
ExecFetchSlotHeapTuple(src) returns shouldFree=true. Latent today (the copy
paths pass buffer/heaptuple slots), but the "nothing should depend on it
now" comment is wrong. Suggest freeing after the insert; that also drops the
slot-type assumption.

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

('tuple' points to something that actually should not need freeing - added a
new assertion and comment.)

Also, could you register this in the open commitfest so cfbot tracks it?

It's registered in the current CF. I think the "needs rebase" status is the
problem. Rebased version attached here.

Testing 0004 now, will post new finding in following emails

Thanks!

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

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

shihao zhong <zhong950419@gmail.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

Antonin Houska <ah@cybertec.at> wrote:

shihao zhong <zhong950419@gmail.com> wrote:

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.

I could spend a little while on this item. Attached is what fixes your
extend.spec.text.

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

Hi Antonin,

Thanks for the diff. I am still on 0004, so the two fixes attached are
against v03-0004 alone. They apply on top of it in this order.

1. replay_all_blocks_the_scan_passed
gap.spec still loses 40 rows with your diff. The replay must use cur, not
the old range end, because the scan skips blocks that have no tuples.

2. syncscan_wraparound
finalize_block_range() only updates range_end, so after the wrap
"blkno < range_start" stays true for every tuple. The check now follows
is_block_in_range(), and the last range is capped at range_start. 100
blocks, scan starts at 48, about 1700 boundaries before, 6 after.

With both, extend.spec and gap.spec lose no rows, and all tests pass.

Thanks,
Shihao

Hi Antonin, Shihao,

shihao zhong <zhong950419@gmail.com> wrote:

I am still on 0004, so the two fixes attached are against v03-0004 alone.

To stay out of Shihao's way, I looked at 0006 and 0008. v03 applies
cleanly on be00f041a33, the master it was made on (on today's master 0004
needs a rebase after 16735af3e39 and a62ff0829d6). Built with
--enable-cassert, plus "use_sort = false" (Shihao's #1), without which
make check crashes in VACUUM FULL. Controls are be00f041a33 without the
series and the series stopped at each patch.

1. 0008: "could not find target tuple" after a concurrent DELETE/UPDATE

REPACK (CONCURRENTLY) fails when a row is deleted or updated by a
transaction that is still in progress when the copy reads the row, and
that commits after the copy and before the changes are replayed. The
attached isolation spec does it on a 1000-row table (two ranges, the
DELETE in the second one) and fails every time; with UPDATE instead of
DELETE it is the same.

Since the failed run leaves the new heap behind (see 2), I could look at
it with pageinspect: it has exactly one tuple with xmax set, the copy of
that row, with the same xmin/xmax as in the old heap (666/668, 668 being
the DELETE). So I think the copy carries the xmax of the transaction
still in progress, and HeapTupleSatisfiesNewHeap() then takes any valid
xmax as committed, so find_target_tuple() skips the row when the DELETE
is replayed.

Without injection points it shows up too. REPACK (CONCURRENTLY) with
default settings on a 30000-row table while another session keeps
deleting rows, 8 runs per build: be00f041a33 8/8 fine, the series up to
0007 8/8 fine, the whole series 4 of 8 failed. (3 runs each with the
series stopped at 0004, 0005 and 0006 were fine as well.) With 0007,
the spec's scenario doesn't get that far: REPACK waits in
RepackWorkerExport while the DELETE's transaction is open. With 0008
it doesn't wait. Shihao's and your diffs don't change this.

2. 0006: the new heap left behind by a failed run blocks other rewrites

The commit message says a failed run leaves the new heap, and the next
REPACK (CONCURRENTLY) drops it. But that cleanup is only in
make_new_heap_for_repack(), and the leftover is pg_temp_<oid> in the
table's own schema, the same name make_new_heap() uses for everything
else. After one failed run (the attached SQL uses an 'error' injection
point), every other rewrite of the table fails until a REPACK
(CONCURRENTLY) succeeds:

VACUUM FULL t; -- all five:
CLUSTER t USING t_pkey; -- ERROR: relation
REPACK t; -- "pg_temp_16395"
ALTER TABLE t ALTER id TYPE bigint; -- already exists
ALTER TABLE t ADD COLUMN w float8 DEFAULT random();

The leftover is a regular table as far as pg_dump knows, and it comes
with a constraint also named t_pkey (on t_pkey_repacknew), so the dump
does not restore cleanly:

CREATE TABLE public.pg_temp_16395 (id integer, v text);
ALTER TABLE ONLY public.pg_temp_16395
ADD CONSTRAINT t_pkey PRIMARY KEY (id);
-- ERROR: relation "t_pkey" already exists

It is the same after a crash: I stopped the server with -m immediate
while REPACK waited at repack-concurrently-before-lock. After recovery
the leftover was there with a full copy of the data (5176 kB for a
4.6 MB table), VACUUM FULL failed, and pg_upgrade to a new cluster
stopped at the same statement:

pg_restore: error: could not execute query: ERROR: relation "t_pkey" already exists

be00f041a33 leaves nothing in the same test, and pg_upgrade succeeds.

I don't know which fix you'd prefer. Doing the lookup of find_new_heaps()
in make_new_heap() would cover all the rewrites, but not pg_dump or
pg_upgrade on a cluster where nobody rewrites the table afterwards.

3. 0008: assertion failure in compute_new_xmax_infomask()

TRAP: failed Assert("TransactionIdIsCurrentTransactionId(add_to_xmax) || !TransactionIdIsValid(GetTopTransactionIdIfAny())"), File: "heapam.c", Line: 5564

It fails in the replay after AccessExclusiveLock, called from
rebuild_relation_finish_concurrent(), in heap_update() of a replayed
UPDATE. So REPACK already has an XID of its own at that point. The
table had only the identity index, so there was no index build before.

A caveat: I only got it on builds with PROGRESS_DEBUG from [1]/messages/by-id/CA+bCEdBKvmoOd=ShLZA99FNHFOc5kjdPRzfOZLgSdcm07uy28g@mail.gmail.com defined,
in 11 of 16 runs (REPACK (CONCURRENTLY) with repack_snapshot_after = 50
on a table that has been repacked once, with INSERT/UPDATE/DELETE
running at the same time). I got it 0 times in 8 runs without that
patch, 8 with log_min_messages = debug2, 6 with a table 10 times larger,
and 8 with the patch applied but PROGRESS_DEBUG not defined. With it,
every tuple the copy scans adds a log line, so my guess is timing, but I
can't rule out that the patch plays a part. Is REPACK expected to have
an XID at that point?

4. Progress reporting

With the trace from [1]/messages/by-id/CA+bCEdBKvmoOd=ShLZA99FNHFOc5kjdPRzfOZLgSdcm07uy28g@mail.gmail.com, these are the phases reported (a table with
only its primary key):

be00f041a33 v03
REPACK (CONCURRENTLY) t 1 7 5 6 8 7 1 5 6 8
... USING INDEX t_pkey 1 3 4 7 5 6 8 7 1 5 7 5 6 8
REPACK t [USING INDEX t_pkey] unchanged

build_new_index() sets PROGRESS_REPACK_PHASE_REBUILD_INDEX and now has
other callers: the identity index of the empty new heap, the one of the
auxiliary table, and the clustering index on the auxiliary table, which
is where the sort happens. So "rebuilding index" shows before "seq
scanning heap", and with USING INDEX "sorting tuples" and "writing new
heap" are never shown. Maybe the phase should only be set where the
table's own indexes are built, and SORT_TUPLES and WRITE_NEW_HEAP
reported around the build and the scan of the auxiliary table's index.

With your diff and Shihao's two (the latter adapted by hand to the whole
series, since they are against 0004 alone), plus use_sort: make check
(239 tests), injection_points (4 + 15) and test_decoding (20 + 14 + 6)
pass, and the trace shows no violations in 154,555 progress changes from
REPACK (CONCURRENTLY) in 50-page ranges, with DML, with synchronized scan
starting mid-table, and with USING INDEX. Without Shihao's syncscan
diff I saw his #3 too: that REPACK went on at about one block every 5 to
10 seconds (439 of 885 after a minute). A small thing: your diff makes
gcc warn that nblocks may be used uninitialized in heapam_handler.c.

[1]: /messages/by-id/CA+bCEdBKvmoOd=ShLZA99FNHFOc5kjdPRzfOZLgSdcm07uy28g@mail.gmail.com

Regards,
Manu

Manu <manuelreyesbravo@gmail.com> wrote:

1. 0008: "could not find target tuple" after a concurrent DELETE/UPDATE

REPACK (CONCURRENTLY) fails when a row is deleted or updated by a
transaction that is still in progress when the copy reads the row, and
that commits after the copy and before the changes are replayed. The
attached isolation spec does it on a 1000-row table (two ranges, the
DELETE in the second one) and fails every time; with UPDATE instead of
DELETE it is the same.

Since the failed run leaves the new heap behind (see 2), I could look at
it with pageinspect: it has exactly one tuple with xmax set, the copy of
that row, with the same xmin/xmax as in the old heap (666/668, 668 being
the DELETE). So I think the copy carries the xmax of the transaction
still in progress, and HeapTupleSatisfiesNewHeap() then takes any valid
xmax as committed, so find_target_tuple() skips the row when the DELETE
is replayed.

When tuple is copied, xmax needs to be set to invalid. If the deleting
transaction commits, it'll set the xmax during replay. I could fix it locally,
will include the fix in the next patch version.

2. 0006: the new heap left behind by a failed run blocks other rewrites

The commit message says a failed run leaves the new heap, and the next
REPACK (CONCURRENTLY) drops it. But that cleanup is only in
make_new_heap_for_repack(), and the leftover is pg_temp_<oid> in the
table's own schema, the same name make_new_heap() uses for everything
else.

Perhaps we need to use more specific name for CONCURRENTLY.

The leftover is a regular table as far as pg_dump knows, and it comes
with a constraint also named t_pkey (on t_pkey_repacknew), so the dump
does not restore cleanly:

CREATE TABLE public.pg_temp_16395 (id integer, v text);
ALTER TABLE ONLY public.pg_temp_16395
ADD CONSTRAINT t_pkey PRIMARY KEY (id);
-- ERROR: relation "t_pkey" already exists

Interesting is that the pg_constraint catalog allows duplicate constraint
name, as long as the constraints are on different relations. Again, the
transient table obviously needs a different constraint name.

3. 0008: assertion failure in compute_new_xmax_infomask()

TRAP: failed Assert("TransactionIdIsCurrentTransactionId(add_to_xmax) || !TransactionIdIsValid(GetTopTransactionIdIfAny())"), File: "heapam.c", Line: 5564

It fails in the replay after AccessExclusiveLock, called from
rebuild_relation_finish_concurrent(), in heap_update() of a replayed
UPDATE. So REPACK already has an XID of its own at that point.

I don't know at the moment when the XID could get assigned. I need to do some
investigation.

4. Progress reporting

With the trace from [1], these are the phases reported (a table with
only its primary key):

be00f041a33 v03
REPACK (CONCURRENTLY) t 1 7 5 6 8 7 1 5 6 8
... USING INDEX t_pkey 1 3 4 7 5 6 8 7 1 5 7 5 6 8
REPACK t [USING INDEX t_pkey] unchanged

build_new_index() sets PROGRESS_REPACK_PHASE_REBUILD_INDEX and now has
other callers: the identity index of the empty new heap, the one of the
auxiliary table, and the clustering index on the auxiliary table, which
is where the sort happens. So "rebuilding index" shows before "seq
scanning heap", and with USING INDEX "sorting tuples" and "writing new
heap" are never shown. Maybe the phase should only be set where the table's
own indexes are built,

Do you mean that we should add variants of WRITE_NEW_HEAP and REBUILD_INDEX
specifically for the auxiliary table?

and SORT_TUPLES and WRITE_NEW_HEAP reported around the build and the scan of
the auxiliary table's index.

With the auxiliary table, sorting IMO hapens in two phases: 1) build the
clustering index and 2) scan the index and insert the output into the new
heap. As long as each phase is reported on its own, I don't see room for
SORT_TUPLES.

A small thing: your diff makes gcc warn that nblocks may be used
uninitialized in heapam_handler.c.

I'll fix that.

[1] /messages/by-id/CA+bCEdBKvmoOd=ShLZA99FNHFOc5kjdPRzfOZLgSdcm07uy28g@mail.gmail.com

Thanks for review, I'll reflect it in the next patch version.

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

Antonin Houska <ah@cybertec.at> wrote:

3. 0008: assertion failure in compute_new_xmax_infomask()

I don't know at the moment when the XID could get assigned. I need to
do some investigation.

I measured it, so you don't have to. gdb attached to the backend
running REPACK, breakpoint on AssignTransactionId(), backtrace on every
hit (script attached). In a whole REPACK (CONCURRENTLY) run there is
exactly one hit, and it is this one:

#0 AssignTransactionId (xact.c:644)
#1 GetCurrentTransactionId (xact.c:461)
#2 LogAccessExclusiveLockPrepare (standby.c:1465)
#3 LockAcquireExtended (lock.c:1008) lockmode=8
#4 LockRelationOid (lmgr.c:115) relid of the new heap
#5 heap_create_with_catalog (heap.c:1294) "pg_temp_16384"
#6 make_new_heap (repack.c:1709)
#7 make_new_heap_for_repack (repack.c:1389)
#8 rebuild_relation (repack.c:1230)

So it is not the replay that assigns the XID, and it is not a race: it
is REPACK itself, creating the transient heap, long before any change
is applied. heap_create_with_catalog() locks the new relation with
AccessExclusiveLock, and LockAcquireExtended() does

if (lockmode >= AccessExclusiveLock &&
locktag->locktag_type == LOCKTAG_RELATION &&
!RecoveryInProgress() &&
XLogStandbyInfoActive())
{
LogAccessExclusiveLockPrepare();
log_lock = true;
}

and LogAccessExclusiveLockPrepare() is a bare
GetCurrentTransactionId(), so that the standby can release the lock at
commit (its comment explains why).

The two thresholds line up exactly: XLogStandbyInfoActive() is
wal_level >= WAL_LEVEL_REPLICA, and repack.c:1012 already refuses to
run with anything below WAL_LEVEL_REPLICA. So the branch is taken on
every REPACK that is allowed to start at all, and the backend has a
top XID from the moment it creates the transient heap.

That makes the Assert unsatisfiable from that side: by the time the
replay calls heap_update() for a change whose xmax belongs to another
transaction, GetTopTransactionIdIfAny() is necessarily valid. It is
not that the XID is assigned too early by accident - there is nowhere
later to move it, short of creating the new heap in a separate
transaction. Whether the right answer is to relax the Assert for this
caller or to change where the XID comes from is your call; I did not
want to guess at it, but the measurement seemed worth having before
you spend time looking.

4. Progress reporting

...
Do you mean that we should add variants of WRITE_NEW_HEAP and
REBUILD_INDEX specifically for the auxiliary table?

Not necessarily - I think there are two separate things in there, and
only the second one is a design question.

The first is a plain reporting bug: build_new_index() sets the phase
for indexes that are not the table's own - the identity index of the
empty new heap and the indexes of the auxiliary table. That alone is
what makes "rebuilding index" appear before "seq scanning heap", and
twice with USING INDEX. Not setting the phase for those internal
builds fixes the order without touching the catalog or the docs.

The second is what the auxiliary table's work should be reported as,
and there I would rather not add new values. Today, with v03 and
USING INDEX, SORT_TUPLES and WRITE_NEW_HEAP are never reported at all,
while the "REPACK Phases" table in monitoring.sgml still says
"REPACK is currently sorting tuples" and "REPACK is currently writing
the new heap". A user watching pg_stat_progress_repack on v19 and on
v20 would see two documented phases disappear.

With the auxiliary table, sorting IMO hapens in two phases: 1) build
the clustering index and 2) scan the index and insert the output into
the new heap. As long as each phase is reported on its own, I don't
see room for SORT_TUPLES.

Your two phases map onto the two existing values, I think: (1) is
where the tuplesort actually runs, so that is SORT_TUPLES, and (2) is
WRITE_NEW_HEAP. That is the room for SORT_TUPLES - the sort is the
index build. It keeps the documented set of phases intact and needs
no catversion bump.

If you do prefer new values for the auxiliary table, that is fine too,
but then monitoring.sgml and system_views.sql have to move with it. I
have a test module for exactly that check (it cross-reads progress.h,
system_views.sql and monitoring.sgml and compares the documented
phases against the ones a real run reports) in [1]https://commitfest.postgresql.org/patch/7331/; I'm happy to run
it against your next version either way, before it becomes someone
else's bug report.

One more thing to keep an eye on: Sami Imseih is changing the same
phases in [2]https://commitfest.postgresql.org/patch/7330/ (v2-0001 docs for v19, v2-0002 phases for v20). Worth
syncing so the two don't collide.

[1]: https://commitfest.postgresql.org/patch/7331/
[2]: https://commitfest.postgresql.org/patch/7330/

Regards,
Manu

Manu <manuelreyesbravo@gmail.com> wrote:

Antonin Houska <ah@cybertec.at> wrote:

3. 0008: assertion failure in compute_new_xmax_infomask()

I don't know at the moment when the XID could get assigned. I need to
do some investigation.

I measured it, so you don't have to. gdb attached to the backend
running REPACK, breakpoint on AssignTransactionId(), backtrace on every
hit (script attached). In a whole REPACK (CONCURRENTLY) run there is
exactly one hit, and it is this one:

#0 AssignTransactionId (xact.c:644)
#1 GetCurrentTransactionId (xact.c:461)
#2 LogAccessExclusiveLockPrepare (standby.c:1465)
#3 LockAcquireExtended (lock.c:1008) lockmode=8
#4 LockRelationOid (lmgr.c:115) relid of the new heap
#5 heap_create_with_catalog (heap.c:1294) "pg_temp_16384"
#6 make_new_heap (repack.c:1709)
#7 make_new_heap_for_repack (repack.c:1389)
#8 rebuild_relation (repack.c:1230)

If this was the reason, the assertion would be pretty easy to reproduce.

...

That makes the Assert unsatisfiable from that side: by the time the
replay calls heap_update() for a change whose xmax belongs to another
transaction, GetTopTransactionIdIfAny() is necessarily valid. It is
not that the XID is assigned too early by accident - there is nowhere
later to move it, short of creating the new heap in a separate
transaction.

This XID assignment is fine - make_new_heap_for_repack() eventually starts a
new transaction, and the reason for it is exactly to get rid of the XID.

4. Progress reporting

...
Do you mean that we should add variants of WRITE_NEW_HEAP and
REBUILD_INDEX specifically for the auxiliary table?

Not necessarily - I think there are two separate things in there, and
only the second one is a design question.

The first is a plain reporting bug: build_new_index() sets the phase
for indexes that are not the table's own - the identity index of the
empty new heap

This one *is* the table's own. Admittedly, if the table is empty, it's not
really index rebuild. Although reporting rebuild for pretty short time is
probably not a serious problem (the user can hardly notice this waiting), we
might want to skip updating the REPACK progress in this case
altogether. Moreover, we can skip the actual build, as an existing comment in
make_new_heap_for_repack() indicates:

* XXX NewHeap is empty - should we pass INDEX_CREATE_SKIP_BUILD?

However, I don't know if REPACK_INDEX_REBUILD_COUNT should then be incremented
or not. If we increment it, we still consider buidling an empty index a
"rebuild". If we don't, the final value of the counter will be wrong. Maybe
increment the counter when the data copying is done, but that seems
unnecessarily complicated.

and the indexes of the auxiliary table.

This might be worth a separate phase: if we report the phase
REPACK_PHASE_REBUILD_INDEX, we should probably also increment the
REPACK_INDEX_REBUILD_COUNT counter. And if indexes on the auxiliary table are
included in the counter, user might be confused to see more index builds than
the number of indexes on his table.

That alone is what makes "rebuilding index" appear before "seq scanning
heap", and twice with USING INDEX. Not setting the phase for those internal
builds fixes the order without touching the catalog or the docs.

I concur with [1]/messages/by-id/CAN12+YKo-vjvdPQts6QnHoB3ET5A2or137oN_FGMviQdtYLN6Q@mail.gmail.com that order is not a problem.

The second is what the auxiliary table's work should be reported as,
and there I would rather not add new values. Today, with v03 and
USING INDEX, SORT_TUPLES and WRITE_NEW_HEAP are never reported at all,
while the "REPACK Phases" table in monitoring.sgml still says
"REPACK is currently sorting tuples" and "REPACK is currently writing
the new heap". A user watching pg_stat_progress_repack on v19 and on
v20 would see two documented phases disappear.

WRITE_NEW_HEAP should be used in process_auxiliary_table() - I'll fix that,
thanks. Regarding SORT_TUPLES, the actual "sorting" is implemented as an index
build and scan, so there's no need to report this phase. And no, the phase
will not disappear from the documentation because REPACK w/o CONCURRENTLY will
still use it.

With the auxiliary table, sorting IMO hapens in two phases: 1) build
the clustering index and 2) scan the index and insert the output into
the new heap. As long as each phase is reported on its own, I don't
see room for SORT_TUPLES.

Your two phases map onto the two existing values, I think: (1) is
where the tuplesort actually runs, so that is SORT_TUPLES, and (2) is
WRITE_NEW_HEAP. That is the room for SORT_TUPLES - the sort is the
index build. It keeps the documented set of phases intact and needs
no catversion bump.

I see your point now, but not sure this is a good idea. If the user sees that
sorting takes too much time, and if he's familiar with PG internals, he's
likely to conclude that increased maintenance_work_mem will help. However, in
this special case it will not because the tuplesort engine is not involved.

[1]: /messages/by-id/CAN12+YKo-vjvdPQts6QnHoB3ET5A2or137oN_FGMviQdtYLN6Q@mail.gmail.com

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

Hi Antonin,

One more for 0008, for your next version.

finalize_block_range() moves *range_start_dst_p before the replay. If the
replay adds pages to the new heap, the next update_identity_index() indexes
those rows again. REPACK then fails with a duplicate key error.

The attached spec hits it every time. The diff sets the range start after
the replay. Both are against v03 plus your 9/18 diff.

Thanks,

Shihao

shihao zhong <zhong950419@gmail.com> wrote:

One more for 0008, for your next version.

finalize_block_range() moves *range_start_dst_p before the replay. If the replay adds pages to the new heap, the next update_identity_index
() indexes those rows again. REPACK then fails with a duplicate key error.

Right, I've noticed that earlier this week when addressing your other
findings. The fix will be included in the next version. Thanks.

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

Antonin Houska <ah@cybertec.at> wrote:

The next version is attached.

Manu <manuelreyesbravo@gmail.com> wrote:

3. 0008: assertion failure in compute_new_xmax_infomask()

TRAP: failed Assert("TransactionIdIsCurrentTransactionId(add_to_xmax) || !TransactionIdIsValid(GetTopTransactionIdIfAny())"), File: "heapam.c", Line: 5564

It fails in the replay after AccessExclusiveLock, called from
rebuild_relation_finish_concurrent(), in heap_update() of a replayed
UPDATE. So REPACK already has an XID of its own at that point.

I don't know at the moment when the XID could get assigned. I need to do some
investigation.

This is still on my TODO list, I'm afraid I couldn't reproduce this problem yet.

4. Progress reporting

With the trace from [1], these are the phases reported (a table with
only its primary key):

be00f041a33 v03
REPACK (CONCURRENTLY) t 1 7 5 6 8 7 1 5 6 8
... USING INDEX t_pkey 1 3 4 7 5 6 8 7 1 5 7 5 6 8
REPACK t [USING INDEX t_pkey] unchanged

build_new_index() sets PROGRESS_REPACK_PHASE_REBUILD_INDEX and now has
other callers: the identity index of the empty new heap, the one of the
auxiliary table, and the clustering index on the auxiliary table, which
is where the sort happens. So "rebuilding index" shows before "seq
scanning heap", and with USING INDEX "sorting tuples" and "writing new
heap" are never shown. Maybe the phase should only be set where the table's
own indexes are built,

Do you mean that we should add variants of WRITE_NEW_HEAP and REBUILD_INDEX
specifically for the auxiliary table?

In 0008, I've added a new counter PROGRESS_REPACK_HEAP_TUPLES_INSERTED_AUX for
inserts into the auxiliary table, and a new phase
PROGRESS_REPACK_PHASE_BUILD_INDEX_AUX, indicating that indexes on the
auxiliary table are being built.

Unfortunately, that introduces a collision with an existing parameter
PROGRESS_CREATEIDX_SUBPHASE, which index AM's use, w/o an option to turn it
off. This reminds me of another patch [1]https://commitfest.postgresql.org/patch/6958/ that tries to fix this problem. I'll
try to update it soon.

This version also addresses problems reported in [2]/messages/by-id/CAGRkXqRH2aEVAibX=nhgb1Z5JZj0b2mG4VrmwZY3BkMQbrs8nQ@mail.gmail.com.

[1]: https://commitfest.postgresql.org/patch/6958/
[2]: /messages/by-id/CAGRkXqRH2aEVAibX=nhgb1Z5JZj0b2mG4VrmwZY3BkMQbrs8nQ@mail.gmail.com

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

Hi Antonin,

Thanks for your update.

I think v04 fixes the finalize_block_range() problem.

The compute_new_xmax_infomask() Assert is easy to hit. The
AccessExclusiveLock in rebuild_relation_finish_concurrent() assigns an
XID, so any UPDATE or DELETE committed while REPACK waits for it fails
on replay. The attached spec does it every time.

If you give the new counter a slot that index builds don't write,
like 13, 0008 does not have to depend on 6958.

Thanks,
Shihao

shihao zhong <zhong950419@gmail.com> wrote:

The compute_new_xmax_infomask() Assert is easy to hit. The
AccessExclusiveLock in rebuild_relation_finish_concurrent() assigns an
XID, so any UPDATE or DELETE committed while REPACK waits for it fails
on replay. The attached spec does it every time.

Ah, I see, the assertion itself is wrong. Thanks.

If you give the new counter a slot that index builds don't write,
like 13, 0008 does not have to depend on 6958.

It'd work, but if [1]https://commitfest.postgresql.org/patch/6958/ gets merged, this workaround would be confusing.

[1]: https://commitfest.postgresql.org/patch/6958/

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