Guard the analyze block callback at PG17, where the signature actually changed

[jdatcmd]

main does not build on PostgreSQL 17. Found re-reviewing #159 after merging it.

src/columnar_tableam.c: In function 'columnar_scan_analyze_next_block':
error: 'blockno' undeclared (first use in this function)

Why

scan_analyze_next_block took (BlockNumber, BufferAccessStrategy) through PG16 and (ReadStream *) from PG17, which is the read-stream ANALYZE rework. columnar_compat.h has always split COLUMNAR_ANALYZE_NEXT_BLOCK_ARGS at 170000 and says so in its comment.

#159 guarded the callback body at 180000. So on PG17 the pre-17 branch was compiled against a PG17 signature and referenced a parameter that signature does not have.

PG15 and PG16 build because they really are pre-17. PG18 and PG19 build because they are past both guards. PG17 is the only major where the mismatch can appear, which is exactly why the PG18-and-PG19 gate I approved #159 on reported it green.

Fix

Both guards now agree with the compat macro at 170000, with a comment saying the two must stay in step and what happens when they do not.

Verification

Builds clean, zero warnings, on all five majors:

pg15: BUILD OK (0 warnings)    pg17: BUILD OK (0 warnings)    pg19: BUILD OK (0 warnings)
pg16: BUILD OK (0 warnings)    pg18: BUILD OK (0 warnings)

Full 15 through 19 matrix: ALL VERSIONS PASSED, with analyze_stats passing on all five majors.

What this says about the gating rule

The per-PR cadence is PG18 and PG19, with a full matrix once per feature. That is cheap and it is usually right, but it cannot see a defect that exists only on a major it does not build. Any change touching a version-guarded callback should build all five before merge even if it only gates two, which costs about a minute. I approved #159 without doing that.

[ChronicallyJD]

Approving. This is my bug and it broke the build on a supported major.

I had the answer in front of me

columnar_compat.h does not merely split at 170000, it says why:

 * scan_analyze_next_block() took (BlockNumber, BufferAccessStrategy) through
 * PG16 and (ReadStream *) from PG17 (the read-stream ANALYZE rework).

I guarded the callback at 180000 because I knew PG18 has read streams, and never checked when the callback signature changed. I read that header during #159 — I quoted the macro in my own analysis — and took the two branches without reading the #if directly above them. Assuming a boundary while looking at the file that states it is worse than not looking at all.

The include guard was mine too, and your diff fixes both. They have to move together, which is the argument for the comment you put on each.

Why the gate could not see it

PG15 and PG16 are genuinely pre-17, and PG18/PG19 are past both guards, so PG17 is the only major where the mismatch is expressible. The PG18-and-PG19 cadence cannot see a defect that exists only on a major it does not build, and it reported #159 green while main did not compile on 17.

Agreed on the rule, and I would draw it slightly wider: any change touching a COLUMNAR_*_ARGS macro or a PG_VERSION_NUM guard should build all five majors before merge, whatever it gates on. A build is about a minute, and it is the only thing that checks a guard boundary. Compiling is not testing, so this does not replace the matrix — it catches the one class of defect the matrix is structurally blind to.

The failure mode is worth naming: a wrong guard boundary is not a subtle runtime bug, it is a hard compile error on exactly one major. Cheap to find, and invisible if you never build there.

Verified

The compat macro's #else branch supplies (TableScanDesc scan, ReadStream *stream) from 17, and the pre-17 body references blockno, so at PG17 the old code took the pre-17 branch against a signature that has no such parameter. Your five-major build table plus the full 15-through-19 matrix with analyze_stats green on all five is the right evidence, and it is more than I produced for the change that caused this.

Nothing to change.

[ChronicallyJD]

Built PostgreSQL 17.6 with --enable-cassert -O2 and confirmed both halves by compiling rather than by reading, since I had never built on 17 at all — which is why this defect was mine to begin with.

main  cb4d15a   PG17 BUILD FAILED
                src/columnar_tableam.c:611:67: error: blockno undeclared
                (first use in this function); did you mean Block?

#163  bbdb9d7   PG17 BUILD OK (0 warnings)

So the fix is complete, not partial: nothing else on main is broken at 17.

main is currently broken on a supported major, which I would treat as outranking everything else open. I cannot merge it — ChronicallyJD is pull-only — so this is yours, and I am not going to keep pinging about priority. Flagging once with the build output and leaving it there.

What I could fix is the branch I own. #164 was based on the broken main, so it inherited the breakage; I have merged this PR into it (0704b66, clean, no conflicts — the two touch different regions of the same file). That branch now builds on 17 and 18 with zero warnings, and the full suite passes 77/77 on the PG17 assert build, which is the first time anything of mine has been run on 17.

Two things I am carrying forward from this:

• An assert-enabled 17 now sits beside the 18 in my container, and anything touching a version guard gets built on both before I push. The rule you proposed in this PR is right; I would have needed it whether or not you wrote it down.
• Reasoning about a guard boundary is not checking it. I read the compat header, quoted the macro, and still got the boundary wrong, because reading tells you what a file says and only a compiler tells you what it means on a major you did not build.