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.

[OffgridwithJD]

I pushed the missing CHANGELOG entry: f989662, docs only

This PR was green on all 12 checks and carried no CHANGELOG.md entry, which is
the one thing between it and mergeable. Rather than flag it a fourth time I wrote
it.

Your approval is on record at 55fed91 and this pushes past it, so here is
exactly what changed: one file, CHANGELOG.md, one entry added under
## [Unreleased] / ### Fixed. No source file, no test file, no behaviour. This
repo does not dismiss a stale review, so please read that as an approval of the
code you already read, not of anything new.

Every claim in the entry is measured, not taken from the summary. The numbers
(heap 1030 of 10,000 sampled, columnar all 10,000, sum(id) the whole table's
sum, SYSTEM the same) are from my own A/B on this branch. One sentence I had
not measured when I drafted it — "an unsampled query on the same table still
takes the custom scan" — I then measured on this branch before committing it:

an unsampled scan still uses the custom scan        PASS
an unsampled aggregate still uses a columnar path   PASS
TABLESAMPLE BERNOULLI is refused                    PASS
the heap control still samples                      PASS
4 passed + 0 failed = 4        (pg18a, .so 461dcce3e823)

test/docs_style.sh is 9/9 on the result.

Two things I still owe this PR from my earlier review, neither addressed by the
commit above and neither blocking in my view:

• sqlstate() returns 00000 when psql cannot connect. I proved that against
  port 1: both control arms pass against a dead server. The heap control also
  asserts only "no error" where its header claims "~1000".
• The header quotes a stale plan node name (Columnar Vector Agg); the actual
  node is PgColumnarScan.

Also worth knowing before either lands: #866 and #871 edit the same three hook
sites (columnar_customscan.c:2527, columnar_vector.c:959 and :1783).
Whichever merges first, the other will need a rebase.

[OffgridwithJD]

The #866 / #871 overlap, measured rather than predicted

I said in my previous comment that these two would conflict with each other. That
was a reading of the diffs, so I ran it. Trial-merging audit/inherit-custom-scan
into audit/tablesample-ignored:

CHANGELOG.md               1 conflict hunk
src/columnar_customscan.c  1 conflict hunk
src/columnar_vector.c      1 conflict hunk

Both PRs add an early return to the same guard block in
PgColumnarSetRelPathlist and to the same place in PgColumnarCreateUpperPaths,
so the second to merge needs a rebase, and the resolution is to keep BOTH returns
rather than either. Neither conflicts with main.

They are independent fixes for independent bugs — rte->tablesample and
rte->inh — so this is a merge-order question, not a design question.

[OffgridwithJD]

Out of draft, and this one is now actually mergeable

@jdatcmd — this was approved by you and then sat unmergeable for a different
reason: GitHub refuses to merge a draft, and every PR on this board was one. It is
no longer a draft.

State at 64cb0c9: 12 of 12 checks green, MERGEABLE, approved. The only commit
since your approval is the CHANGELOG entry I described above, which touches nothing
but CHANGELOG.md.

The two things I would still like fixed are in my earlier comment and neither
blocks a merge in my view: sqlstate() returns 00000 when psql cannot connect,
so both control arms pass against a dead server; and the header quotes a stale plan
node name. The one thing to decide before merging is order — #871 touches the same
three hook sites, and the trial merge I ran gives three conflict hunks, so whichever
goes second needs a rebase that keeps both early returns.

#871 is also out of draft now, 12/12 green and MERGEABLE, but it has no review yet.