Pin REPACK's actual behaviour, and name it in the error (#399)

[jdatcmd]

Closes #399. @ChronicallyJD for review. No benchmark needed.

The finding changed the deliverable

You expected a test. What the test found is that the design document's conclusion is
wrong, so this PR is a correction plus the test.

design/PG18_19_OPPORTUNITIES.md said REPACK "should work" because it dispatches
through relation_copy_for_cluster and pgColumnar implements that callback. The
callback is registered and is a stub:

pgcolumnar_relation_copy_for_cluster(...)
{
	COLUMNAR_UNSUPPORTED("CLUSTER / VACUUM FULL");
}

"We implement that callback" was true of the symbol and false of the behaviour. That is
exactly the gap you said a suite closes and a grep does not, and it is why your instinct
to distrust reasoning-about-dispatch was right.

Measured on 19beta2

command	columnar	heap (control)
REPACK r	ERROR	ok
REPACK r USING INDEX	ERROR	-
REPACK (VERBOSE) r	ERROR	-
REPACK CONCURRENTLY r	syntax error	syntax error
REPACK (CONCURRENTLY) r	error	same error on heap
VACUUM FULL / CLUSTER	ERROR	ok
pgcolumnar.vacuum('r')	ok	-

Two things that change your third bullet: REPACK CONCURRENTLY is not the syntax
(it is REPACK (CONCURRENTLY), an option), and that form fails on heap too, so it
is not a columnar behaviour. The suite records it rather than attributing it to us.

The actual user-visible defect

A 19 user types REPACK and is told CLUSTER / VACUUM FULL is not supported yet: two
commands they did not type, and on 19 the ones REPACK replaced. The message now names
REPACK on 19 and hints at pgcolumnar.vacuum().

This is a spelling difference, not a missing capability -- the operation is
available under another name -- so I did not implement relation_copy_for_cluster
under this issue.

The suite

test/native_repack.sh, registered, 19-gated with a visible skip below it (verified
on PG18: SKIP REPACK requires PostgreSQL 19, exit 0).

It asserts the behaviour that exists, not the behaviour that was hoped for: every
spelling fails, the message names the command, the table is unharmed afterwards (rows,
content hash, storage options, access method), and pgcolumnar.vacuum() succeeds and
preserves content. If someone later implements the callback these checks fail, and
that is the intended signal to rewrite them to assert success.

Proven by removal: on unmodified main only the "names REPACK" check fails. Every
other check passes on both builds, because they pin behaviour rather than detect the
change.

Docs

• docs/limitations.md lists REPACK beside CLUSTER and VACUUM FULL.
• CHANGELOG.md entry for the message.
• The design document is corrected in place, marked and dated, rather than quietly.
  It is what produced the wrong expectation, and leaving it would produce it again.

Gate

Five-major matrix, ALL VERSIONS PASSED, native_repack=PASS on all five.

🤖 Generated with Claude Code

[ChronicallyJD]

The finding is right and the correction is overdue. One change: the CONCURRENTLY case pins the wrong reason.

Verified everything else independently on the bench, both majors, before this.

What holds

The version gate works, exercised on both sides of the #if:

pg19a  VACUUM FULL r   ERROR: columnar: REPACK, CLUSTER and VACUUM FULL are not supported yet
pg18a  VACUUM FULL r   ERROR: columnar: CLUSTER and VACUUM FULL are not supported yet

The hint makes a promise, so I tested it rather than trusting it. On both majors, after
deleting half the rows, pgcolumnar.vacuum() succeeds, leaves 25,000 rows and the table
undamaged. The "spelling difference, not a missing capability" claim is accurate.

REPACK h on heap succeeds on the same build, so the control does its job.

The correction to PG18_19_OPPORTUNITIES.md is the right shape, and marking the entry
CORRECTED in place rather than deleting it is better than what I would have done.

The change I am asking for

REPACK (CONCURRENTLY) failed on both heap and columnar in your table, and you concluded
it "fails on heap too, so it is not a columnar behaviour" and asserted the error without
investigating further.

That conclusion holds only because the fixture has no primary key. The error says so:

ERROR:  cannot execute REPACK (CONCURRENTLY) on relation "r"
HINT:  Relation "r" has no identity index.

That is a precondition, not a verdict. With a primary key on 19beta2, wal_level=logical:

  REPACK (CONCURRENTLY) hk  [heap]      ok, no error
  REPACK (CONCURRENTLY) ck  [columnar]  ERROR: columnar: REPACK, CLUSTER and VACUUM FULL
                                               are not supported yet

Heap succeeds. Columnar fails, and it fails with our error. So it is a columnar
behaviour after all, and the reason we could not see that is that the fixture never met
the precondition.

Two consequences:

1. The suite currently pins "REPACK (CONCURRENTLY) errors because there is no identity
   index". Add a primary key to that fixture and the error changes, the test still
   passes, and it now means something different. A test whose meaning depends on an
   incidental property of its fixture is the kind that survives the thing it was written
   to catch.
2. The PR body and the changelog say this form is not ours. On a table that can actually
   be repacked concurrently, it is.

Suggested: a second fixture with a primary key, asserting that heap succeeds and
columnar raises our error, with the no-PK case kept as it is and labelled as the
precondition check. That distinguishes the two failures, which is the whole point.

Good news that came out of the same run

Your fourth bullet worried about a half-finished concurrent rewrite. It does not happen:
after the failed REPACK (CONCURRENTLY), the columnar table still had all 20,000 rows
with every value intact. Worth asserting, since you were right to worry and the answer is
reassuring.

Requesting changes only for the fixture. Everything else I would merge as is.

[jdatcmd]

You were right, and the check was passing for the wrong reason

Fixed and pushed. This is the good kind of catch: my assertion held, and it held
because of a property of the fixture rather than a property of the code.

I wrote that REPACK (CONCURRENTLY) "fails on columnar and on heap alike, so it is not
ours". Both did fail. They failed because the fixture had no identity index, which
PostgreSQL enforces before the access method is ever consulted. Add a primary key and
my check keeps passing while meaning something entirely different, which is exactly
the failure mode I have been flagging in other people's tests this week.

Reproduced your result in my container, with wal_level=logical and room in the
lock table:

control: with an identity index, REPACK (CONCURRENTLY) succeeds on heap   PASS
and on a columnar table it raises OUR error                              PASS

So it is a columnar behaviour. Corrected in three places, since I had it wrong in all
of them: the suite, the changelog, and the PG18_19_OPPORTUNITIES.md note.

What the suite asserts now

Both fixtures, and the assertion is the distinction rather than the outcome:

• without an identity index, the refusal is PostgreSQL's own precondition error and
  explicitly not ours (the check fails if it sees our message there);
• with one, heap succeeds and columnar raises ours.

A single "it errors" check conflated those two, which was the whole defect.

And your good news is pinned

After the refused concurrent repack the columnar table is undamaged: rows and content
hash unchanged. You were right that the fourth bullet on #399 was worth worrying about
and right that the answer is reassuring, so it is now asserted rather than observed.

Two environmental preconditions, which are the same trap one level down

The suite now sets wal_level=logical and raises max_locks_per_transaction. Without
them REPACK (CONCURRENTLY) is unreachable for reasons that have nothing to do with
the access method, and I would have been pinning the wrong failure again. The first
run without the lock headroom died with out of shared memory on CREATE TABLE ... PRIMARY KEY, which is how I found it.

19 checks, all passing on PG19, visible skip below it.

Also merged main in

#408 landed while this was in review, and both branches touch the SUITES array.
Resolved keeping native_ctas and native_repack, and the matrix is re-running on the
merged tree rather than the pre-merge one, since a stale tree under test is a false
green.

Will post that result here when it finishes. Ready for another look.

[ChronicallyJD]

Approved. The new assertion is better than what I suggested.

I proposed a second fixture asserting that heap succeeds and columnar raises ours. You did
that and added the part I missed: the no-identity-index case now fails if it sees our
message there. So the suite asserts the distinction rather than the outcome, and a single
"it errors" check can no longer stand in for two different reasons.

That is the correct shape. My version would still have passed if our error had started
appearing on the precondition path.

Correcting it in all three places, including the design note, is right. A test that pins
the truth while a document beside it states the opposite is how the REPACK claim survived
in the first place.

One collision to sort out, not a review objection

We are both editing design/PG18_19_OPPORTUNITIES.md right now.

My #411 renames it to design/POSTGRESQL_VERSION_ADOPTION.md and rewrites item 5 with
your finding, because the old name pinned the file to two majors. Your #409 corrects item 5
in place under the old name.

Whichever merges second will conflict, and a rename against an edit is the kind git
resolves badly.

Suggestion: merge this one first. It is the correctness change and it should not wait
on a documentation rename. I will rebase #411 on top, take your wording for item 5 over
mine where they differ, and carry it into the renamed file. That way your text is the one
that survives and I do the reconciling.

Say if you would rather it went the other way and I will hold #411 instead.

Verified

Your fix reproduces what I measured: with an identity index, REPACK (CONCURRENTLY)
succeeds on heap and raises our error on columnar. Pinning the undamaged-table result is
worth having, since the worry that prompted it was reasonable.

[jdatcmd]

Matrix on the merged tree (with #408 in it), five majors:

  PASS   PG15   PASS   PG16   PASS   PG17   PASS   PG18   PASS   PG19
ALL VERSIONS PASSED

native_ctas=PASS    x5
native_repack=PASS  x5

Both suites green on every major, and native_repack skips visibly below 19 rather than passing silently. Ready to merge when you are.