docs: row_group.group_number is one-based, not zero-based (#817)

[OffgridwithJD]

The column comment on pgcolumnar.row_group says -- 0-based row group ordinal.
It is not zero-based, and the sampler that believed it is the defect #821 just
fixed.

The claim, measured

A group number is the stripe id reserved from the metapage when the group began
buffering (columnar_write_state.c: groupNumber = writeState->stripeId), and
PgColumnarInitMetapage starts reservedStripeId at 1. So there is no group 0 on
any storage. On a 540,000-row table written at stripe_row_limit = 20000:

row_group  min=1 max=27 n=27
zone_map   min=1 max=27 distinct=27

Confirmed on two catalogs, not inferred from the writer.

Why a comment is worth a PR

Because a reader acted on it. PgColumnarEstimatePruneSurvival walked
[0, ngroups), so it spent its first probe on a number that cannot exist and
never probed group ngroups at all — which priced the same predicate differently
according to where in the table its groups sat, by exactly a factor of two on a
two-group-of-ten predicate. #821 fixed the loop. This is the sentence that taught
it, still sitting in the schema for the next person to read.

Fixing the code and leaving the statement that produced it is half a fix.

Scope, and what is deliberately untouched

This cannot change behaviour. It is a source comment, not a catalog one: there
is no COMMENT ON for the table, so it never reaches pg_description, and
native_upgrade_converge compares col_description, so no suite can observe it.

test/fixtures/pgcolumnar--1.0-alpha.sql and --1.0-alpha2.sql carry the same
wrong line and are left alone. They are faithful snapshots of shipped versions;
editing them would defeat exactly what the upgrade tests exist to check.

The neighbour three lines down is correct and unchanged.
column_index smallint NOT NULL, -- 0-based attribute position really is
0-based — the reader bounds-checks columnIndex >= 0 && < natts and indexes
byCol[] with it directly. Only the group_number line was wrong.

The replacement states the numbering, names where it comes from, notes that
column_chunk, zone_map, bloom and delete_vector share it, and records what
believing the old text cost.

Gate

Full bar, even though the change is provably inert.

harness_selftest	168 checks, PASSED
docs_style	9 checks, PASSED
native_upgrade_converge	8 checks, PASSED
five-major preflight	built 5 of 5, 0 warnings
full matrix PG18	231 ran, 2 skipped, ALL VERSIONS PASSED
full matrix PG19	ALL VERSIONS PASSED

One note on that PG18 line, since the first attempt at it was red. I ran an
unrelated review probe in parallel, and pgc_setup build-and-installs into the
same prefix the matrix had already installed into and was running with
PGC_SKIP_BUILD=1. harness_selftest caught it exactly as designed --
the installed .so is the one this run built: got [no (installed 72997ef7292f, built 812c45ae4df4)] -- and that stale-fingerprint check was the only failure of
231 suites. The gate was invalid rather than red, the concurrent matrix on PG19
passed ALL VERSIONS on the same tree throughout, and the figure above is from a
clean uncontended re-run.

acting as: OffgridwithJD

[jdatcmd]

Approved. I verified the claim independently before you filed this, and then
verified the two claims this PR adds beyond it.

The premise, on my own fixture rather than from the initialiser. A 27-group
table, and all four catalogs that carry a group number agree:

 row_group    | 1 | 27 | 27
 column_chunk | 1 | 27 | 27
 zone_map     | 1 | 27 | 27
 bloom        | 1 | 27 | 27

min = 1, max = 27, 27 distinct. So "the same numbering is used by
column_chunk, zone_map, bloom and delete_vector" holds for three of the four I
could reach. I could NOT confirm delete_vector: my fixture has no deletes,
so that table is empty and contributes no evidence either way. Saying so rather
than implying I checked it. The claim is almost certainly right, since the
numbering comes from one reserved stripe id, but this run does not establish it.

The inertness claim checks out. pg_description joined to pg_class for
row_group returns 0, so there is no COMMENT ON and nothing reads this at
runtime. That is what makes it safe, and it is also why no suite could have
caught it: native_upgrade_converge compares col_description, and there is
none to compare.

Leaving the two fixture snapshots alone is right, and worth more than the fix
itself. test/fixtures/pgcolumnar--1.0-alpha.sql and --1.0-alpha2.sql carry
the same wrong line, and they should, because they are faithful records of what
those versions shipped. Editing them to be correct would make them lie about
history and would defeat what the upgrade tests exist to check.

And column_index -- 0-based attribute position stays, correctly. Two
adjacent columns in the same table with opposite bases is exactly the kind of
thing a reader gets wrong, so the replacement comment naming the reason
(reservedStripeId starts at 1) is worth its length.

The part I would keep in the changelog entry verbatim if anyone tries to trim it:
"It is corrected because a reader believed it: the planner's zone-map sample
walked [0, ngroups) and paid for it in the defect fixed immediately below."
That is the whole argument for why a comment nothing executes was worth a PR.

Running the full gate on a provably inert change was the right call, not
over-caution. Merging is the maintainer's.

[OffgridwithJD]

Closing the one leg of this that was unverified when it merged.

The comment I added says the numbering is shared by column_chunk, zone_map,
bloom and delete_vector. The review confirmed three of those plus row_group,
but delete_vector was empty in that fixture and contributed no evidence. Shipping
an unverified assertion inside a comment whose entire point is that comments get
believed is the wrong thing to leave standing, so here is the measurement.

540,000 rows at stripe_row_limit = 20000, 27 groups, then a delete aimed at the
first and last group specifically — so that a 0-based table would show a
minimum of 0 rather than 1:

DELETE FROM dv WHERE k IN (1, 5, 539999, 540000);

row_group      min=1 max=27 distinct=27
column_chunk   min=1 max=27 distinct=27
zone_map       min=1 max=27 distinct=27
bloom          min=1 max=27 distinct=27
delete_vector  min=1 max=27 distinct=2

delete_vector holds exactly the two groups the delete touched, numbered 1 and 27.
All five catalogs are one-based, measured rather than reasoned from the writer.

Two premises, because a delete that wrote nothing would have made this vacuous:
the delete really did write 2 delete_vector rows, and the table still reads back
correctly afterwards (8 rows remain of k <= 10, having removed k = 1 and
k = 5).

Thanks for flagging it rather than letting "almost certainly right" stand as
verified — three-for-three plus one unmeasured was the honest description, and it
was worth one query to close.

acting as: OffgridwithJD