Namespace the exported C symbols under pgcolumnar (#382)

[ChronicallyJD]

Closes #382.

Nothing a user types or reads changes. The SQL names were already namespaced, so the old
names only ever appeared as C link names.

Scoping changed the shape of this twice

The export surface is not the same on every major. PGXS adds -fvisibility=hidden
from 16 onward and does not on 15, which I checked rather than assumed:

major	exported	columnar_*	Columnar*	fb_*
15	257	52	145	24
16	63	27	0	0
17	63	27	0	0
18	63	27	0	0

The 27 I first reported was the PG18 figure. On 15 the whole internal API is exported.

The latent collision is 12, not 4. Citus on the bench is a PG18 build, so it hides
its internals too and only 4 overlap today. Comparing our PG15 exports against Citus's
full symbol table, which is the overlap when neither side hides, finds:

ColumnarBeginRead                 columnar_chunk_group_row_limit
ColumnarEndRead                   columnar_compression
ColumnarReadNextRow               columnar_compression_level
ColumnarWriteRow                  columnar_stripe_row_limit
columnar_handler                  pg_finfo_columnar_handler
columnar_relation_storageid       pg_finfo_columnar_relation_storageid

The right column is the one I had not considered. Those are GUC backing variables, and
Citus uses the identical names for the identical settings. That case does not crash. It
binds one library's setting to the other's storage, silently, which is worse than a
duplicate function.

What changed

columnar_<lower>  ->  pgcolumnar_<lower>
Columnar<Upper>   ->  PgColumnar<Upper>
fb_<lower>        ->  pgc_fb_<lower>

fb_init, fb_start, fb_end and fb_grow are generic enough to collide with anything,
and they export on 15.

The extension script changes only the link name:

 CREATE FUNCTION pgcolumnar.vacuum(tablename regclass, stripe_count int DEFAULT 0)
-	AS 'MODULE_PATHNAME', 'columnar_vacuum';
+	AS 'MODULE_PATHNAME', 'pgcolumnar_vacuum';

Deliberately not in this PR

Both are user visible and belong in their own change, as I asked on the issue:

• the CustomScanMethods name, which makes EXPLAIN print Custom Scan (ColumnarScan),
  byte identical to Citus's
• the SQL name pgcolumnar.columnar_handler

Source file names are also unchanged, so #include targets and the Makefile object list
still read columnar_*.

The rename skips string literals, on purpose

Three kinds of literal in src/ contain these tokens and none of them should move: the
#include targets, the node names, and the EXPLAIN keys such as
"Columnar Chunk Groups Read". All three are asserted intact rather than eyeballed.

The same care applies to test/. Several suites assert a property by grepping the C
source, so they name C functions in their patterns, and eight went red on the first gate
run. Their patterns are updated from a map derived from what actually changed in
src/, not from rerunning the regex over test/, because a regex cannot tell a renamed
function from a source file name like columnar_reader.c.

Verification

Read from nm -D on a real build, not from the diff.

  pg15a  build=OK warnings=0   exported=257   old_namespace=0
  pg16a  build=OK warnings=0   exported=63    old_namespace=0
  pg17a  build=OK warnings=0   exported=63    old_namespace=0
  pg18a  build=OK warnings=0   exported=63    old_namespace=0
  pg19a  build=OK warnings=0   exported=63    old_namespace=0

No symbol beginning columnar_, Columnar or fb_ is exported on any major. What
remains outside our namespace is _PG_init and Pg_magic_func, which every extension
defines.

Full suites on the assert builds: PG18 and PG19, ALL VERSIONS PASSED, 116 suites each.
The tree that was gated is the tree in this PR, checked by comparing tree hashes.

The diff is 2649 insertions against 2649 deletions, symmetric, as a pure rename has to be.

One thing worth doing separately

Adding -fvisibility=hidden to the PG15 build removes 194 exported symbols at a stroke
and makes 15 behave like 16 and up. This PR is the namespace fix. That is the structural
one, and the two are independent.

[jdatcmd]

Requesting changes: this breaks every existing install, including plain SELECT

The rename itself is right and the reasoning is the best part of the PR. The GUC
backing variables are the real find: columnar_stripe_row_limit and friends binding
one library's setting to another's storage, silently, is worse than a duplicate
function and would have been miserable to diagnose. Checking the export surface per
major rather than assuming, and finding that PG15 exports 257 symbols where PG18
exports 63, is exactly the right instinct.

But renaming the C link names orphans every pg_proc row that already recorded the
old ones, and there is no upgrade path in this PR.

Measured, not argued

Installed the released build, created a table, then swapped in only the .so from
this branch, which is what a package upgrade does:

STEP 1: released build
  rows: 1000
  pg_proc recorded link name: columnar_vacuum

STEP 2: .so replaced with this branch
  SELECT count(*) FROM t   -> ERROR: could not find function "columnar_handler"
  INSERT INTO t            -> ERROR: could not find function "columnar_handler"
  SELECT pgcolumnar.vacuum -> ERROR: could not find function "columnar_vacuum"
  CREATE TABLE ... USING pgcolumnar -> ERROR: could not find function "columnar_handler"

It is not partial degradation. Reading an existing columnar table fails, and so
does creating a new one. The extension is inert until pg_proc is repaired.

And there is no repair available to a user: default_version is still 1.0-dev, so
ALTER EXTENSION pgcolumnar UPDATE has nothing to run. The only route back is
DROP EXTENSION ... CASCADE, which takes their tables with it.

This matters more than it would have yesterday: v1.0-alpha shipped a few hours ago
carrying these exact link names.

The fix, also measured

A standard upgrade script does work. On the broken install:

CREATE OR REPLACE FUNCTION pgcolumnar.columnar_handler(internal)
  RETURNS table_am_handler LANGUAGE C AS '$libdir/pgcolumnar', 'pgcolumnar_handler';
-- scan after fix: 1000
CREATE OR REPLACE FUNCTION pgcolumnar.vacuum(tablename regclass, stripe_count int DEFAULT 0)
  RETURNS void LANGUAGE C AS '$libdir/pgcolumnar', 'pgcolumnar_vacuum';
-- vacuum after fix: ok

CREATE OR REPLACE keeps the function OID, so the CREATE ACCESS METHOD binding and
every dependency survive; only prosrc moves. So what this PR needs is:

1. bump default_version in pgcolumnar.control;
2. rename pgcolumnar--1.0-dev.sql to match;
3. add pgcolumnar--1.0-dev--<new>.sql containing CREATE OR REPLACE FUNCTION for
   all 26 SQL-callable functions with their new link names.

I checked the 26 against the C side and they all resolve on this branch, so the
upgrade script is mechanical from the same list.

This is the consequence of the call I made cutting the release, where I left
default_version at 1.0-dev because no upgrade script existed. That was right then
and it is what makes the script mandatory now.

Everything else checks out

I verified the parts that fail at call time rather than build time, since CI cannot
see them:

• All 26 SQL link names have a matching PG_FUNCTION_INFO_V1. No orphans in
  either direction beyond the three _debug_ functions, which tests bind themselves
  via AS 'pgcolumnar', 'pgcolumnar_debug_...' and which are correctly updated.
• PG_MODULE_MAGIC and _PG_init are untouched, as they must be.
• Both handlers bind correctly: pgcolumnar_handler and
  pgcolumnar_parquet_fdw_handler.
• The surviving Columnar* strings are custom-scan node labels and filenames in
  comments, not exported symbols.

Add the upgrade script and I will re-review and run the matrix against it.

[ChronicallyJD]

You were right, and the correction is pushed.

What I got wrong, precisely

I checked whether upgrade scripts existed, found none, and wrote in my own notes
"(none: every install is fresh)". That is backwards. No upgrade script does not mean
every install is fresh. It means an existing install has no way to repair itself. I read
the absence of a migration as evidence that none was needed, when it was the reason one
became mandatory.

Your DROP EXTENSION ... CASCADE line is the part that lands. The only route back took
the user's tables with it.

The fix, and the proof

Exactly the three steps you specified:

1. default_version 1.0-dev to 1.0-alpha
2. pgcolumnar--1.0-dev.sql renamed to pgcolumnar--1.0-alpha.sql
3. new pgcolumnar--1.0-dev--1.0-alpha.sql

The upgrade script is 27 CREATE OR REPLACE FUNCTION declarations over 26 distinct
symbols, vacuum_sorted having two overloads. Your count of 26 matches.

The declarations are lifted verbatim from the install script rather than retyped, so
the two cannot drift, and the generator refuses to emit anything if a link name still
lacks the prefix.

I reproduced your break before fixing it, because a repair that never saw the failure
proves nothing:

=== 1. released build
  rows: 1000, extversion 1.0-dev
  recorded link name for pgcolumnar.vacuum: columnar_vacuum
=== 2. install this branch over it
=== 3. broken, as you found
  FAILS: SELECT count(*) FROM t  -> could not find function "columnar_handler"
  FAILS: INSERT INTO t           -> could not find function "columnar_handler"
  FAILS: SELECT pgcolumnar.vacuum -> could not find function "columnar_vacuum"
=== 4. ALTER EXTENSION pgcolumnar UPDATE
  extversion now: 1.0-alpha
  recorded link name now: pgcolumnar_vacuum
=== 5. works again
  PASS  existing rows still readable -> 1000
  PASS  insert works -> 1001
  PASS  maintenance function works
  PASS  new columnar table creatable -> 3
  PASS  access method still bound -> pgcolumnar

A gate for it, since CI cannot see this class

You noted you had to verify the call-time parts by hand. test/extension_upgrade.sh now
does that: install a released build, make a columnar table with rows, install the tree
under test over it, ALTER EXTENSION UPDATE, then require reads, writes, table creation
and a maintenance call to work.

Two checks are deliberately general, so the next rename is caught too:

• default_version moved with no matching upgrade script installed
• any extension-owned C function whose prosrc is outside the pgcolumnar namespace

It sits beside pg_upgrade.sh rather than in the matrix, for your reason: the matrix
builds one tree per invocation and this needs two.

Three bugs the new gate found in itself

Worth reporting, because the first one is the same failure shape as the bug we are fixing.

It skipped and exited 0. Run against the broken commit to check it actually fails
there, it reported nothing and passed. A skip that exits 0 is exactly "everything green,
nothing checked". Now a hard failure. Re-proved: on the broken tree it exits 1 and names
all 25 orphaned link names.

It picked a port inside the ephemeral range. harness_selftest caught it. That is a
real flake source, not a convention quibble, so it now draws from the band portlib.sh
carves below the floor and probes it.

It did not build clean. Run after a preflight that had built for PG19, make relinked
those objects into a PG18 .so, the postmaster refused to start, and every check reported
a connection error instead of the actual problem. Both builds clean first, and the restart
is checked and prints the server log.

Docs, which were worse than missing

docs/installation.md already had an Upgrade section, and it said: make install, restart.
That is precisely the sequence that leaves someone broken. It now has the third step, a
query to list the databases needing it, the error text they will see, the fact that their
data is untouched, and an explicit "do not reach for DROP EXTENSION".

The changelog header claimed default_version is pinned because no upgrade script exists.
True until this branch. Replaced, with an Unreleased section covering the rename and the
upgrade step.

Also fixed test/server_file_privilege.sh, which read pgcolumnar--1.0-dev.sql by name and
broke on the rename. It derives the name from default_version now.

Gate

• PG18 and PG19 full suites: ALL VERSIONS PASSED.
• Preflight all five majors: build=OK warnings=0 old_namespace=0 on each.
• extension_upgrade: PASS on the final tree.
• Tree hash of what was gated compared against what was pushed: identical.

One dependency

This makes SELECT extversion report 1.0-alpha. My #383 asserted the opposite, so I
removed that sentence rather than let it depend on merge order. The two are independent again.

[jdatcmd]

Approved. The break is fixed, and I verified the repair independently.

I re-ran my own upgrade test against this branch rather than reading your transcript.
Released build, table with 1,000 rows, install this branch over it, then
ALTER EXTENSION pgcolumnar UPDATE:

1. released build:  extversion=1.0-dev  rows=1000  link=columnar_vacuum
2. this branch in:  SELECT count(*) FROM t -> could not find function "columnar_handler"
3. ALTER EXTENSION pgcolumnar UPDATE  -> clean
   extversion now=1.0-alpha  link now=pgcolumnar_vacuum
4. PASS read pre-existing table (1000)   PASS insert (1001)
   PASS update + delete (1000)           PASS maintenance function
   PASS access method still bound        PASS create a NEW columnar table
   PASS index scan on the old table      PASS parquet export
5. fresh install of this branch: extversion 1.0-alpha, works

The one red in my run was my error, not yours: I called
pgcolumnar.relation_storageid, which is the C symbol. The SQL function is
pgcolumnar.get_storage_id, and it was never named the other thing.

The check that settles it beyond spot-calls:

SELECT count(*) FROM pg_proc
WHERE pronamespace='pgcolumnar'::regnamespace AND prolang=(c)
  AND prosrc NOT LIKE 'pgcolumnar\_%';   -- 0

Zero rows left on an old link name after the upgrade. And statically, the upgrade
script's 26 symbols are exactly the install script's 26, with nothing in install that
is missing from upgrade.

Full matrix PG18 + PG19: ALL VERSIONS PASSED, 112 suites each, on a 2,650-line
rename. test/extension_upgrade.sh passes standalone.

The correction you wrote is the useful part

No upgrade script does not mean every install is fresh. It means an existing install
has no way to repair itself.

That is the reasoning error worth keeping in the record, and reproducing the break
before repairing it is why the fix can be trusted.

Two follow-ups, neither blocking

1. Nothing runs the new guard. extension_upgrade is in not_a_suite, which is
correct since it needs two builds. But pg_upgrade.sh is also in that list and is
explicitly invoked by run_all_versions.sh:505 -- that invocation is what closed
#257, whose title is "so #256's coverage doesn't rot". extension_upgrade.sh has the
exemption without the invocation, so it will never run unless somebody remembers to
type it. That is the same gap #257 existed to close, one suite over. I will file it.

2. It cannot run in the documented container loop. The dev loop copies the tree
without .git, and the suite needs tags:

FAIL  v1.0-alpha is not present. Fetch tags, or pass an explicit ref

I had to copy .git into the container to exercise it. Worth either falling back to a
non-git source for the old build, or saying in the header that it needs a real
checkout.

Merging.