From 75f8c8f98c790a6a68f7abef48c47169956bb9fc Mon Sep 17 00:00:00 2001 From: talmacschen-arch Date: Fri, 30 Oct 2026 02:17:07 +0800 Subject: [PATCH 1/2] Quote comma-containing storage option values so PAX tables restore GetTableStorage builds a table's WITH clause by joining pg_class.reloptions with ", ". A single reloption is stored as a "name=value" element, and some PAX storage options (minmax_columns, cluster_columns) hold a comma-separated column list as their value. Joining the array verbatim makes the commas inside such a value indistinguishable from the separator between options, so the restored DDL ... USING pax WITH (minmax_columns=a,b, cluster_columns=a,b, ...) fails with `ERROR: unrecognized parameter "b"` and the table is never created. Its data load, ALTER ... OWNER, indexes and ANALYZE then all cascade into "relation does not exist", so the table is lost on restore. Single-quote the value of any reloption whose value contains a comma so the list survives as one value. Options without a comma are emitted unchanged, so backup output for existing tables is unaffected. This mirrors the existing single-quoting in buildStorageOptionsString for storage servers. Add an integration test that backs up a PAX table with multi-column minmax_columns/cluster_columns and asserts the emitted WITH clause restores verbatim. --- backup/queries_table_defs.go | 16 +++++++++++- .../predata_table_defs_queries_test.go | 26 +++++++++++++++++++ 2 files changed, 41 insertions(+), 1 deletion(-) diff --git a/backup/queries_table_defs.go b/backup/queries_table_defs.go index 3b5d1351..9c55bf65 100644 --- a/backup/queries_table_defs.go +++ b/backup/queries_table_defs.go @@ -602,10 +602,24 @@ func GetPartitionAlteredSchema(connectionPool *dbconn.DBConn) map[uint32][]Alter func GetTableStorage(connectionPool *dbconn.DBConn) (map[uint32]string, map[uint32]string) { gplog.Info("Getting storage information") + // A single reloption is stored as a "name=value" element. Some storage + // options (e.g. PAX minmax_columns/cluster_columns) carry a comma-separated + // list as their value, so the value itself contains commas. A plain + // array_to_string(reloptions, ', ') would make those embedded commas + // indistinguishable from the separator between options, and the restored + // DDL fails with `unrecognized parameter ""`. Single-quote + // the value of any option whose value contains a comma so the list is + // preserved as one value on restore. Options without a comma are emitted + // unchanged to keep backup output stable. query := fmt.Sprintf(` SELECT c.oid, quote_ident(t.spcname) AS tablespace, - array_to_string(reloptions, ', ') AS reloptions + array_to_string(ARRAY( + SELECT CASE WHEN position(',' in opt) > 0 + THEN left(opt, position('=' in opt) - 1) || '=' || quote_literal(substr(opt, position('=' in opt) + 1)) + ELSE opt + END + FROM unnest(reloptions) AS opt), ', ') AS reloptions FROM pg_class c JOIN pg_namespace n ON c.relnamespace = n.oid LEFT JOIN pg_tablespace t ON t.oid = c.reltablespace diff --git a/integration/predata_table_defs_queries_test.go b/integration/predata_table_defs_queries_test.go index 79a5d4ed..c4446ad3 100644 --- a/integration/predata_table_defs_queries_test.go +++ b/integration/predata_table_defs_queries_test.go @@ -846,6 +846,32 @@ SET SUBPARTITION TEMPLATE Expect(result[oid]).To(Equal("blocksize=32768, compresslevel=0, compresstype=none, checksum=true")) } }) + It("single-quotes comma-containing storage option values so PAX multi-column options restore correctly ", func() { + // PAX minmax_columns/cluster_columns store a comma-separated column + // list as a single string value. If that value is emitted unquoted, + // the embedded comma is read as an option separator and the restored + // DDL fails with `unrecognized parameter ""`. The value + // must be single-quoted so it survives the round trip. + if !connectionPool.Version.IsCBDB() { + Skip("PAX access method is only available on Cloudberry") + } + testhelper.AssertQueryRuns(connectionPool, + "CREATE TABLE public.pax_mm(a int, b int, c int) USING pax WITH (minmax_columns='a,b', cluster_columns='a,b', cluster_type='lexical') DISTRIBUTED BY (a)") + defer testhelper.AssertQueryRuns(connectionPool, "DROP TABLE public.pax_mm") + oid := testutils.OidFromObjectName(connectionPool, "public", "pax_mm", backup.TYPE_RELATION) + + _, result := backup.GetTableStorage(connectionPool) + + // The comma-containing list values must be single-quoted ... + Expect(result[oid]).To(ContainSubstring("minmax_columns='a,b'")) + Expect(result[oid]).To(ContainSubstring("cluster_columns='a,b'")) + + // ... and the emitted WITH clause must restore verbatim. This asserts + // the wiring end to end: a failure to quote would make this CREATE fail. + testhelper.AssertQueryRuns(connectionPool, + fmt.Sprintf("CREATE TABLE public.pax_mm_restored(a int, b int, c int) USING pax WITH (%s) DISTRIBUTED BY (a)", result[oid])) + testhelper.AssertQueryRuns(connectionPool, "DROP TABLE public.pax_mm_restored") + }) }) Describe("GetTableInheritance", func() { child := backup.Relation{Schema: "public", Name: "child"} From 77cf51b995a4ee3800cfb529f1de5fe6bff9661e Mon Sep 17 00:00:00 2001 From: talmacschen-arch Date: Sat, 31 Oct 2026 23:30:36 +0800 Subject: [PATCH 2/2] Also quote non-token (uppercase/spaced) storage option values The initial fix single-quoted only option values containing a comma. A value that is not a plain token breaks the round trip even without a comma, verified on a live Cloudberry 2.1.0 cluster: - an uppercase/quoted identifier is silently downcased (minmax_columns=A targets column "a"); - a value containing a space is a syntax error (bloomfilter_columns=x y). Quote whenever the value is not a simple token (not matching ^[a-z0-9_.]+$), which also covers the comma case. Plain-token values (compresstype=zstd, compresslevel=5, fillfactor=70, checksum=true, ...) are emitted unchanged, so backup output for existing heap/AO tables stays byte-for-byte identical. WITH ORDINALITY preserves the original option order. Add an integration test for the uppercase/spaced cases asserting the emitted WITH clause restores verbatim. Note: PAX materialized views (GetMaterializedViews in queries_relations.go) build their WITH clause the same way and have the same defect; that path is left for a follow-up PR to keep this change focused on base tables. --- backup/queries_table_defs.go | 32 +++++++++++-------- .../predata_table_defs_queries_test.go | 27 ++++++++++++++++ 2 files changed, 46 insertions(+), 13 deletions(-) diff --git a/backup/queries_table_defs.go b/backup/queries_table_defs.go index 9c55bf65..d759348a 100644 --- a/backup/queries_table_defs.go +++ b/backup/queries_table_defs.go @@ -602,24 +602,30 @@ func GetPartitionAlteredSchema(connectionPool *dbconn.DBConn) map[uint32][]Alter func GetTableStorage(connectionPool *dbconn.DBConn) (map[uint32]string, map[uint32]string) { gplog.Info("Getting storage information") - // A single reloption is stored as a "name=value" element. Some storage - // options (e.g. PAX minmax_columns/cluster_columns) carry a comma-separated - // list as their value, so the value itself contains commas. A plain - // array_to_string(reloptions, ', ') would make those embedded commas - // indistinguishable from the separator between options, and the restored - // DDL fails with `unrecognized parameter ""`. Single-quote - // the value of any option whose value contains a comma so the list is - // preserved as one value on restore. Options without a comma are emitted - // unchanged to keep backup output stable. + // A single reloption is stored as a "name=value" element and the WITH clause + // is built by joining them with ", ". A value that is not a plain token + // (lowercase letters, digits, '_' or '.') breaks that round trip: + // - PAX minmax_columns/cluster_columns carry a comma-separated column list, + // e.g. "minmax_columns=a,b"; the embedded comma is read as the separator + // between options and restore fails with `unrecognized parameter "b"`. + // - an identifier with uppercase or special characters is silently + // downcased ("minmax_columns=A" targets column "a"), and a value with a + // space is a syntax error ("minmax_columns=x y"). + // Single-quote the value of any option whose value is not a simple token so + // it survives verbatim on restore. Plain-token values (compresstype=zstd, + // compresslevel=5, fillfactor=70, checksum=true, ...) are emitted unchanged, + // so backup output for existing tables is byte-for-byte unaffected. WITH + // ORDINALITY preserves the original option order. query := fmt.Sprintf(` SELECT c.oid, quote_ident(t.spcname) AS tablespace, array_to_string(ARRAY( - SELECT CASE WHEN position(',' in opt) > 0 - THEN left(opt, position('=' in opt) - 1) || '=' || quote_literal(substr(opt, position('=' in opt) + 1)) - ELSE opt + SELECT CASE WHEN substr(opt, position('=' in opt) + 1) ~ '^[a-z0-9_.]+$' + THEN opt + ELSE left(opt, position('=' in opt) - 1) || '=' || quote_literal(substr(opt, position('=' in opt) + 1)) END - FROM unnest(reloptions) AS opt), ', ') AS reloptions + FROM unnest(reloptions) WITH ORDINALITY AS u(opt, n) + ORDER BY n), ', ') AS reloptions FROM pg_class c JOIN pg_namespace n ON c.relnamespace = n.oid LEFT JOIN pg_tablespace t ON t.oid = c.reltablespace diff --git a/integration/predata_table_defs_queries_test.go b/integration/predata_table_defs_queries_test.go index c4446ad3..b6160564 100644 --- a/integration/predata_table_defs_queries_test.go +++ b/integration/predata_table_defs_queries_test.go @@ -872,6 +872,33 @@ SET SUBPARTITION TEMPLATE fmt.Sprintf("CREATE TABLE public.pax_mm_restored(a int, b int, c int) USING pax WITH (%s) DISTRIBUTED BY (a)", result[oid])) testhelper.AssertQueryRuns(connectionPool, "DROP TABLE public.pax_mm_restored") }) + It("single-quotes storage option values that are not simple tokens (uppercase, spaces) so they restore correctly", func() { + // A storage option value that is not a plain token also breaks the + // round trip even without a comma: an uppercase identifier is silently + // downcased (minmax_columns=A targets column "a"), and a value with a + // space is a syntax error (bloomfilter_columns=x y). Both must be + // single-quoted so the value survives verbatim on restore. + if !connectionPool.Version.IsCBDB() { + Skip("PAX access method is only available on Cloudberry") + } + testhelper.AssertQueryRuns(connectionPool, + `CREATE TABLE public.pax_q(a int, "A" int, "x y" int) USING pax WITH (minmax_columns='A', bloomfilter_columns='x y') DISTRIBUTED BY (a)`) + defer testhelper.AssertQueryRuns(connectionPool, "DROP TABLE public.pax_q") + oid := testutils.OidFromObjectName(connectionPool, "public", "pax_q", backup.TYPE_RELATION) + + _, result := backup.GetTableStorage(connectionPool) + + // The non-token values must be single-quoted ... + Expect(result[oid]).To(ContainSubstring("minmax_columns='A'")) + Expect(result[oid]).To(ContainSubstring("bloomfilter_columns='x y'")) + + // ... and the emitted WITH clause must restore verbatim onto the same + // uppercase/spaced columns. Without quoting this CREATE would downcase + // "A" to a missing column or fail with a syntax error. + testhelper.AssertQueryRuns(connectionPool, + fmt.Sprintf(`CREATE TABLE public.pax_q_restored(a int, "A" int, "x y" int) USING pax WITH (%s) DISTRIBUTED BY (a)`, result[oid])) + testhelper.AssertQueryRuns(connectionPool, "DROP TABLE public.pax_q_restored") + }) }) Describe("GetTableInheritance", func() { child := backup.Relation{Schema: "public", Name: "child"}