[talmacschen-arch]

Problem

GetTableStorage builds a table's WITH (...) clause by joining pg_class.reloptions with , :

array_to_string(reloptions, ', ') AS reloptions

A reloption is stored as a single name=value array element. The PAX storage options minmax_columns / cluster_columns hold a comma-separated column list as their value, so the value itself contains commas. Joining the array verbatim makes those embedded commas indistinguishable from the , separator between options, and the restored DDL

... USING pax WITH (minmax_columns=a,b, cluster_columns=a,b, cluster_type=lexical, compresstype=zstd)

fails:

ERROR: unrecognized parameter "b" (SQLSTATE 22023)

The table is then never created, so its data load, ALTER ... OWNER, indexes and ANALYZE all cascade into relation does not exist — the table and its data are lost on restore. Observed on Apache Cloudberry 2.1.0 restoring PAX tables with multi-column minmax_columns.

The PAX kernel stores these as a single string reloption and splits the value on commas itself (add_string_reloption + SplitDirectoriesString(value, ',')), so the value must reach the server single-quoted as one string.

Fix

Single-quote the value of any reloption whose value contains a comma. Options without a comma are emitted unchanged, so backup output for existing tables is unaffected. This mirrors the single-quoting already done in buildStorageOptionsString for storage servers.

Testing

Added an integration test in the GetTableStorageOptions block that creates a PAX table with multi-column minmax_columns/cluster_columns and asserts the emitted WITH clause (a) single-quotes the list values and (b) restores verbatim via a real CREATE TABLE. Gated to Cloudberry, where PAX is available.

[talmacschen-arch]

Cluster validation

Verified on a live Apache Cloudberry 2.1.0 cluster with the patched gpbackup/gprestore.

A PAX table with multi-column list options was created and populated:

CREATE TABLE public.pmm(a int, b int, c int)
  USING pax WITH (minmax_columns='a,b', cluster_columns='a,b') DISTRIBUTED BY (a);
INSERT INTO public.pmm SELECT i, i%10, i%3 FROM generate_series(1,1000) i;

• Backup (patched): the emitted DDL carried the single-quoted list value — ... WITH (minmax_columns='a,b', ...).
• Restore into a fresh database succeeded, and the table came back with all its data: 1000 rows backed up → 1000 restored.

Kernel side (contrib/pax_storage/.../paxc_rel_options.cc) confirms why the quoting is required: minmax_columns/cluster_columns are registered via add_string_reloption and the value is split on , internally (SplitDirectoriesString(value, ',', ...)), so the comma-separated list must reach the server as one single-quoted string.

Without this fix, GetTableStorage emits the value unquoted (minmax_columns=a,b); the restored CREATE TABLE then fails with ERROR: unrecognized parameter "b" (observed in production as "ws_order_number" / "account_id"), the table is never created, and its data, owner, indexes and ANALYZE all cascade into relation does not exist — i.e. the table is lost on restore.

[liang8283]

Thanks for tracking this down — the root cause analysis is spot on, and the fix works for regular tables. I verified it end to end on a Cloudberry cluster with a full gpbackup → gprestore round trip, building both main and this branch:

Object	main	this PR
PAX table, minmax_columns='a,b', cluster_columns='a,b', ...	❌ unrecognized parameter "b", table + data lost	✅ all rows restored
PAX table, bloomfilter_columns='a,b', minmax_columns='b,c'	❌	✅
PAX partitioned table (parent + leaf partition)	❌	✅
PAX table, minmax_columns='A,B c' (mixed-case / space in names)	❌	✅
PAX table, minmax_columns='a' (single column)	✅	✅ unchanged output
PAX materialized view, minmax_columns='a,b'	❌	❌ still fails

The new integration test runs (not skipped) and passes on that cluster, and go test ./backup/... passes.

A few things I'd like to see addressed before merge:

1. Materialized views are not covered (blocking)

GetMaterializedViews builds its WITH clause the same way, in backup/queries_relations.go (both version6Query and atLeast7Query):

coalesce(' WITH (' || array_to_string(c.reloptions, ', ') || ')', '') AS options,

So a PAX materialized view still produces a broken WITH clause:

CREATE MATERIALIZED VIEW public.pax_mv USING pax WITH (minmax_columns=a,b) AS ...
ERROR: unrecognized parameter "b" (SQLSTATE 22023)

followed by ALTER MATERIALIZED VIEW ... OWNER failing with relation does not exist. Could the reloption formatting be pulled into one shared SQL snippet (e.g. a Go helper that returns the expression for a given column) and used in all three places, plus a matching integration test for the matview case?

2. Quoting only on , leaves closely related cases broken (suggestion)

Any value that is not a plain lowercase identifier/number has the same problem, even without a comma. Both of these fail on main and on this branch:

minmax_columns='A'    -> WITH (minmax_columns=A)    -> ERROR: invalid column name 'a'   (identifier gets downcased)
minmax_columns='x y'  -> WITH (minmax_columns=x y)  -> ERROR: syntax error at or near "y"

These aren't regressions from this PR, but since it's the same root cause it seems worth fixing together. Something like quoting whenever the value isn't a simple token keeps the output for existing tables byte-for-byte unchanged:

array_to_string(ARRAY(
	SELECT CASE WHEN substr(opt, strpos(opt, '=') + 1) ~ '^[a-z0-9_.]+$'
		THEN opt
		ELSE split_part(opt, '=', 1) || '=' || quote_literal(substr(opt, strpos(opt, '=') + 1))
	END
	FROM unnest(reloptions) WITH ORDINALITY AS u(opt, n)
	ORDER BY n), ', ')

I tried this on the same cluster against PAX (minmax_columns='a,b', cluster_columns='A', bloomfilter_columns='x y'), AO column (compresstype=zlib, compresslevel=1, blocksize=32768, checksum=true), heap (fillfactor=70, autovacuum_enabled=false) and a PAX matview. The generated WITH clauses all re-create cleanly, and the recreated relations' reloptions are identical to the originals. Unquoted options (compresstype=zstd, compresslevel=5, checksum=true, ...) are emitted exactly as before.

[talmacschen-arch]

Thanks for the thorough review and the cross-cluster verification!

Point 2 (non-token values) — fixed in this PR. I reproduced both cases on a live Cloudberry 2.1.0 cluster:

• minmax_columns='x y' (space) → ERROR: syntax error at or near "y" on restore;
• minmax_columns='A' on a quoted "A" column → emitted unquoted it silently downcases to column a, so the min/max index is built on the wrong column.

The quoting condition is now "quote unless the value is a simple token", i.e. it quotes whenever substr(value) !~ '^[a-z0-9_.]+$', which also subsumes the comma case. WITH ORDINALITY preserves option order. I confirmed existing heap/AO options (compresstype=zstd, compresslevel=5, fillfactor=70, checksum=true, ...) are emitted byte-for-byte identical to before, and that the new uppercase/spaced values restore verbatim (added an integration test for them).

Point 1 (materialized views) — deferred to a follow-up PR. I confirmed the matview defect is real (GetMaterializedViews emits WITH (minmax_columns=a,b) → unrecognized parameter "b"). To keep this PR focused on base tables I've left GetMaterializedViews (queries_relations.go) for a separate PR and added a note to that effect in the commit message. I'll open it with a shared formatting helper and a matview integration test as you suggested.

[talmacschen-arch]

Follow-up on point 1 (materialized views): the materialized-view path is now fixed in a dedicated PR — #130 ("Quote non-token storage option values for materialized views"). It applies the same quoting to GetAllViews (both the GPDB6 and GPDB7+/Cloudberry queries) via a shared helper, so a PAX matview with minmax_columns/cluster_columns no longer fails restore with unrecognized parameter. Verified end to end on a live Cloudberry 2.1.0 cluster.

For completeness, incremental materialized views (IVM) are handled in #131, which builds on the same GetAllViews query.

This PR (#127) stays scoped to base tables as discussed.

[woblerr]

1. GPDB 5 compatibility

WITH ORDINALITY is not supported by GPDB 5, but version validation still accepts GPDB 5.1+. The query has no version guard.

Two options:

• Keep a compatible query for GPDB 5.
• Drop GPDB 5 support and update validation, tests, and docs.

@tuhaihe What do you think? Do we still need code-level compatibility with GPDB 5?

2. The updated regex still misses numeric column names

The updated regex handles uppercase names and spaces from the earlier review, but leaves 001 unquoted. This turns minmax_columns='001' into minmax_columns=001, which SQL parses as 1, changing the target column. Could string-valued options always be quoted?

BEGIN;

CREATE TABLE src ("001" int, "1" int)
USING pax WITH (minmax_columns='001')
DISTRIBUTED BY ("001");

-- WITH clause emitted by this PR:
CREATE TABLE dst ("001" int, "1" int)
USING pax WITH (minmax_columns=001)
DISTRIBUTED BY ("001");

SELECT relname, reloptions
FROM pg_class
WHERE oid IN ('src'::regclass, 'dst'::regclass);
-- Expected: src has minmax_columns=001, dst has minmax_columns=1.

ROLLBACK;

The parser conversion is confirmed; the PAX example still needs a live cluster check.

[talmacschen-arch]

Reworked to address the review. Instead of the comma-only regex, this now mirrors the server's own get_reloptions() (ruleutils.c) in one shared helper, relOptionsClause().

@liang8283

• Materialized views are covered now. The formatting is pulled into relOptionsClause() and used in all three places — GetTableStorage and both the GPDB6 and GPDB7+/Cloudberry variants of GetAllViews — so a PAX matview with minmax_columns/cluster_columns restores. Added an integration spec that creates a PAX matview with a comma value, asserts the quoted output and restores the emitted WITH clause.
• Point 2 (non-token values) falls out of the same rule: a value is left bare only when quote_ident(value) = value, otherwise it's a quoted literal — so A, x y, numbers and reserved words are all quoted, matching pg_dump.

@woblerr

• Numeric values (point 2): fixed. 001 is not a bare identifier, so it's emitted as minmax_columns='001', not =001. Added a spec that restores onto a "001" column and checks the restored reloptions still read 001, not 1.
• GPDB 5 (point 1): the query no longer uses WITH ORDINALITY. Ordering is done with generate_series over the array subscripts, and LATERAL is avoided too, so it runs on GPDB 5 (PostgreSQL 8.3) with no version guard.

The output now matches pg_dump byte for byte, which does change existing backups: scalar numbers and reserved words are quoted (fillfactor='70', compresstype='none', checksum='true'). The parser accepts both forms; the hardcoded test expectations are updated to match.

Verified end to end on a live Apache Cloudberry 2.1.0 cluster: the GetTableStorage/GetAllViews specs (comma, non-token, numeric, PAX matview, view/matview options) and the create-statement specs all pass, and round-tripped tables' reloptions match the originals.

This also folds in #130, which I'll close as covered here.

[woblerr]

LGTM
Both issues are resolved.