fix: refuse TABLESAMPLE rather than returning every row

[OffgridwithJD]

Summary

• Planner hooks now decline a columnar path when rte->tablesample is set, so TABLESAMPLE reaches the AM sample callbacks.
• Those callbacks already raise 0A000. The custom scan and vectorized aggregate paths used to replace Sample Scan and return every row.

Test coverage

• test/tablesample.sh

Made with Cursor

[OffgridwithJD]

Review of the fix and the suite. I did not run the branch itself; both findings
below are measured on the tree I have, and I say which is which.

The hook coverage is complete — verified, not assumed

The claim that these three call sites are the whole surface holds. Every hook the
extension installs:

columnar_customscan.c:3692  set_rel_pathlist_hook   = PgColumnarSetRelPathlist
columnar_vector.c:5769      create_upper_paths_hook = PgColumnarCreateUpperPaths
columnar_tableam.c:3166..84 object_access, ProcessUtility, ExecutorEnd,
                            build_simple_rel, get_relation_info

and every place an RTE is read to decide on a columnar path:

columnar_vector.c:958   PgColumnarCreateUpperPaths
columnar_vector.c:1780  PgColumnarTryGroupAggPath
columnar_customscan.c   PgColumnarSetRelPathlist

Those are exactly the three the PR patches. The remaining hooks do not create
paths. Nothing is missing.

sqlstate() reports success when psql cannot connect — measured

The helper returns 00000 whenever its sed finds no ERROR: XXXXX: line, and
a connection failure produces no such line. Replicating the helper verbatim
against a dead port:

against a dead server on port 1, the helper reports: [00000]

So both control arms pass against a server that never answered:

check "control: an unsampled count reports success, ..." "$st_ok"   "00000"
check "control: the same TABLESAMPLE on a heap table raises nothing" "$st_heap" "00000"

This is not a false green for the suite — the five 0A000 arms would go red and
the run would fail visibly. But the two arms whose job is to exclude "the probe
is broken" are themselves satisfied by the broken-probe case, so they do not do
that job.

The heap arm is the weaker of the two, because the header states a stronger fact
than the arm asserts:

#     heap:    ~1000
#     columnar: 10000, plan: Custom Scan (Columnar Vector Agg)

"heap: ~1000" is the observation that makes the whole PR meaningful — TABLESAMPLE
really samples, so returning 10000 is really wrong — and no arm pins it. The arm
only asserts that no error was raised.

Suggested: assert the heap sample is a strict subset, which pins the probe AND
the sampling in one arm.

h_n="$(q 'SELECT count(*) FROM h TABLESAMPLE BERNOULLI(10) REPEATABLE (7)')"
check "control: heap TABLESAMPLE really samples, so 10000 would be wrong" \
    "$([ -n "$h_n" ] && [ "$h_n" -gt 0 ] && [ "$h_n" -lt 10000 ] && echo yes)" "yes"

-gt 0 matters as much as -lt 10000: an empty result and a dead connection both
give the empty string, and neither should pass.

Not verified by me

I have not reproduced the premise — that count(*) ... TABLESAMPLE BERNOULLI(10)
returned 10000 on a columnar table before this change. It is the load-bearing
claim of the PR and the one thing I would want pinned by a run rather than by the
header comment. I will measure it separately and follow up here.

Reviewed as OffgridwithJD. I am not approving this: the PR is authored by the same
account, and a self-approval is exactly what suspended self-merging was for.

[OffgridwithJD]

Follow-up: I said I would measure the premise rather than take it from the header.
Measured on pg18a, 10,000 rows, shipped defaults:

columnar total rows               : 10000
heap     TABLESAMPLE BERNOULLI(10): 1030
columnar TABLESAMPLE BERNOULLI(10): 10000
columnar TABLESAMPLE SYSTEM(10)   : 10000
columnar sum(id) sampled          : 50005000

The premise holds. The heap control samples (1030 of 10000), the columnar table
returns every row for both BERNOULLI and SYSTEM, and docs/limitations.md
promises an error.

One thing worth adding to the PR: sum(id) over the sampled scan returns
50005000, which is exactly sum(1..10000). So this is not only a wrong row
count -- the vectorized aggregate folds the entire table and returns a wrong
answer to an aggregate query. That is a stronger statement of the defect than
"returns every row", and I would put it in the header, because it is the version
a reader will care about.

A stale plan-node name in the header

The header says:

#     columnar: 10000, plan: Custom Scan (Columnar Vector Agg)

What I observe is:

Custom Scan (PgColumnarScan)
  Columnar Vectorized Aggregates: 1

Columnar Vector Agg is not the node name any more -- the scan was renamed when
it collided with TimescaleDB's registry entry. The name in the comment is one
nobody can grep for. Since the suite deliberately asserts SQLSTATE rather than
plan text, nothing breaks; it is the comment that is wrong, and comments that
quote output are worth pinning to output that exists.

With the premise confirmed and the hook coverage verified complete, the two
remaining points from my earlier comment stand: the 00000-on-connection-failure
behaviour of sqlstate(), and the heap control asserting only "no error" where
the header claims "~1000".

Reviewed as OffgridwithJD. Not approving -- same account as the author.

[jdatcmd]

Reviewed adversarially at 55fed91. Approving. This is the best-built PR in the current batch and I want to say why before the one finding: it asserts SQLSTATE instead of message text, states the reason in the file ("a grep for 'not supported' is also satisfied by a dozen unrelated errors, and by a connection failure"), and carries two controls rather than one.

I ran the removal proof the PR does not carry, and all three guards are load-bearing

Each guard reverted alone, on pg18_assert, 8 checks every arm:

mutation	reds	which arms
baseline	0	—
revert the SetRelPathlist guard	4	sum, SYSTEM, projected, grouped
revert the CreateUpperPaths guard	3	count(*), sum, SYSTEM
revert the TryGroupAggPath guard	1	grouped

Every guard has at least one arm that reddens, and the signatures differ — count(*) reds under the upper-paths guard but not the pathlist guard, and grouped TABLESAMPLE is the only arm pinning TryGroupAggPath at all. That last row is worth keeping in mind if anyone ever trims this suite: delete that one arm and a whole hook goes unguarded.

Worth pasting into the PR body. A reviewer should not be the first person to find out whether each half of a three-part fix is tested.

One attack of mine that failed, reported because a negative result is evidence too

I went looking for a fourth planner hook left unguarded. There isn't one. PgColumnarPlanCustomPath and PgColumnarPlanAggPath have no tablesample check and correctly do not need one — they are plan-time callbacks over a path already chosen, so they cannot introduce the Sample Scan replacement. All three path-adding hooks are guarded. My first sweep said otherwise and that was my analysis window being too small, not a hole in the PR.

MINOR: sqlstate() has no unparsed bucket

if [ -n "$code" ]; then printf '%s\n' "$code"; else printf '00000\n'; fi

When the helper cannot extract a code it returns 00000 — success, a real outcome, rather than "I could not tell". That is the parser-with-no-unparsed-bucket shape: every parse failure becomes a plausible answer.

It fails safe for the five 0A000 arms, which go red. It does not fail safe for the two controls:

control: an unsampled count reports success, so the probe is not stuck on 0A000
control: the same TABLESAMPLE on a heap table raises nothing

Both pass whenever the probe cannot parse — including against a server that never answered, which is the exact case the file's own comment says it wants to exclude. A third bucket fixes it:

if [ -n "$code" ]; then printf '%s\n' "$code"
elif printf '%s' "$out" | grep -q '^ERROR'; then printf 'UNPARSED\n'
else printf '00000\n'; fi

Then a control asserting 00000 means "the statement succeeded" rather than "nothing matched my regex".

Not blocking: the five arms that carry the fix are proved falsifiable above, and the controls are belt-and-braces on top of them. Worth fixing before the next suite copies the helper, which is how expect_error spread to eleven files.

CI 12 of 12, MERGEABLE/CLEAN.