Repository navigation
Quote reloption values that are not bare identifiers - #135
Merged
Merged
Conversation
Contributor
Author
|
The dependency PR is created at #136, so let PR 136 be merged first. |
gfphoenix78
force-pushed
the
fix-reloptions
branch
from
November 6, 2026 14:49
b75cdbd to
bfb6e0c
Compare
A backup of a PAX table carrying bloomfilter_columns restored with
ERROR: unrecognized parameter "v2"
from the dump's own DDL:
) USING pax WITH (bloomfilter_columns=v1,v2) DISTRIBUTED BY (id);
pg_class.reloptions is an array of "name=value" strings, and a value is
free to contain a comma. PAX has three options that always do --
minmax_columns, bloomfilter_columns and cluster_columns all hold
comma-separated column lists. Flattening the array with
array_to_string(reloptions, ', ') loses the element boundaries, so one
option comes back out as several and the restore fails on the
fragments. The backup itself succeeds, so the damage only surfaces when
someone tries to restore.
Flatten reloptions the way the server does it instead. get_reloptions()
in ruleutils.c splits each element on its first '=', quotes the name as
an identifier, and leaves the value bare only when it is already an
identifier needing no quoting -- otherwise it emits a quoted literal.
Mirroring that rule means a value we quote is a value the server would
have quoted, and the output now matches pg_dump byte for byte:
pg_dump : WITH (minmax_columns='a,b', bloomfilter_columns='c,d', cluster_type=lexical)
gpbackup: WITH (minmax_columns='a,b', bloomfilter_columns='c,d', cluster_type=lexical)
pg_dump : WITH (fillfactor='50', autovacuum_enabled='false')
gpbackup: WITH (fillfactor='50', autovacuum_enabled='false')
That rule quotes more than just the comma case: numbers and reserved
words are quoted too, because quote_ident() does not leave them bare.
'none', 'true' and '32768' are all now quoted, which is what the server
emits and what the parser accepts either way.
Applied to the three places that flatten a reloptions array into a
WITH (...) clause: GetTableStorage for tables, and both version variants
of GetAllViews, since a materialized view can be PAX and carry the same
options. NULLIF keeps the previous NULL-when-empty behaviour, which the
callers depend on -- they wrap the result in coalesce(' WITH (' || ...)
and would otherwise emit an empty " WITH ()" on every relation with no
storage options.
The existing expectations that hardcoded the unquoted forms are updated
to the server-faithful ones.
Tests: two integration specs on GetTableStorage, one for a value with a
comma and one pinning the bare-vs-quoted rule. The storage matrix fixture
now gives its PAX table minmax_columns and bloomfilter_columns, so the
full backup-and-restore round trip covers it; reverting the fix makes
that spec fail with the original error.
Full integration run compared against a7ecb50: both have the same 11
failures, no additions and no removals, so nothing here regressed. The
11 are environment gaps on this cluster, chiefly a missing gps3ext.so.
The end_to_end suite failed on the REL_2_STABLE CI job with 47 failures,
all of which trace back to one error:
ERROR: operator is not unique: unknown || oid (SQLSTATE 42725)
HINT: Could not choose a best candidate operator. You might need to
add explicit type casts.
getPAXAuxRelations() finds a PAX table's aux relation by name, building
the name from the table's oid:
ON aux_c.relname::text IN ('pg_manifest_' || paxtables.oid, ...)
An untyped literal next to an oid leaves the parser two candidates it
cannot choose between: text || anynonarray, reading the literal as text,
and anycompatiblearray || anycompatible, reading it as an array. The
operators themselves are identical between the two servers -- the two
pg_operator.dat files match -- so what differs is how each kernel
resolves that tie. PostgreSQL 16, which Cloudberry 3.x is built on,
picks one; PostgreSQL 14, which Cloudberry 2.x is built on, reports the
ambiguity and fails.
That is why none of this reproduces on a 3.x development cluster, and
why the first sign of it was a CI job.
Type the literal instead of leaving it to inference. Once the left side
is text the array reading is gone, and only one candidate is left:
text || text would need an implicit cast from oid, or from integer in
the fixtures below, and neither exists. The remaining candidate,
text || anynonarray, renders the right side through its output function
and produces the same string as before.
backup/queries_incremental.go the aux relation lookup, in
both the IN list and the CASE
that tells the two layouts
apart -- 36 of the failures,
every spec that passes
--leaf-partition-data
end_to_end/rewrite_detection_test.go 'v' || i and 'r' || i, which
end_to_end/storage_matrix_test.go populate the fixtures -- 8
more failures, this time
while building the tables
rather than backing them up
integration/incremental_queries_test.go the same aux lookup, in the
assertion that the marker
query reads the right table
The remaining 7 failures are a consequence rather than a separate
problem. They report one more relation than expected in restoredb, and
testdb is created once in BeforeSuite and never reset, so a spec killed
partway through leaves its tables behind for every later backup to pick
up.
end_to_end_suite_test.go carries the same shape in
setupGpbackmanFilterDatabase(), predating this work. It happened to run
before the failing backup in that BeforeEach rather than after it, so
the job never reached a point where it mattered; type it too rather than
leave the question open.
Checked the rest of the tree for the same pattern. The surviving
concatenations are safe for a reason, not by luck: quote_ident() and
substr() return text, the pg_pax_blocks columns are already cast
explicitly, and the manifest layout's path column is text in the
catalog.
Verified on a 3.x cluster that the rewritten lookup is equivalent -- the
query returns the same four rows and both directions of EXCEPT are
empty -- and that a backup still records marker, last DDL timestamp and
relfilenode for every AO and PAX table. The ambiguity itself cannot be
reproduced there, so the fix needs a 2.x CI run to confirm.
gfphoenix78
force-pushed
the
fix-reloptions
branch
2 times, most recently
from
November 6, 2026 16:33
b75cdbd to
572569c
Compare
Member
|
Hi @gfphoenix78, we need to make sure all the tests pass. There are some failed tests, please create another PR on them. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A backup of a PAX table carrying bloomfilter_columns restored with
from the dump's own DDL:
pg_class.reloptions is an array of "name=value" strings, and a value is free to contain a comma. PAX has three options that always do -- minmax_columns, bloomfilter_columns and cluster_columns all hold comma-separated column lists. Flattening the array with array_to_string(reloptions, ', ') loses the element boundaries, so one option comes back out as several and the restore fails on the fragments. The backup itself succeeds, so the damage only surfaces when someone tries to restore.
Flatten reloptions the way the server does it instead. get_reloptions() in ruleutils.c splits each element on its first '=', quotes the name as an identifier, and leaves the value bare only when it is already an identifier needing no quoting -- otherwise it emits a quoted literal. Mirroring that rule means a value we quote is a value the server would have quoted, and the output now matches pg_dump byte for byte:
That rule quotes more than just the comma case: numbers and reserved words are quoted too, because quote_ident() does not leave them bare. 'none', 'true' and '32768' are all now quoted, which is what the server emits and what the parser accepts either way.
Applied to the three places that flatten a reloptions array into a WITH (...) clause: GetTableStorage for tables, and both version variants of GetAllViews, since a materialized view can be PAX and carry the same options. NULLIF keeps the previous NULL-when-empty behaviour, which the callers depend on -- they wrap the result in coalesce(' WITH (' || ...) and would otherwise emit an empty " WITH ()" on every relation with no storage options.
The existing expectations that hardcoded the unquoted forms are updated to the server-faithful ones.
Tests: two integration specs on GetTableStorage, one for a value with a comma and one pinning the bare-vs-quoted rule. The storage matrix fixture now gives its PAX table minmax_columns and bloomfilter_columns, so the full backup-and-restore round trip covers it; reverting the fix makes that spec fail with the original error.
Full integration run compared against a7ecb50: both have the same 11 failures, no additions and no removals, so nothing here regressed. The 11 are environment gaps on this cluster, chiefly a missing gps3ext.so.
closes: #ISSUE_Number
Change logs
Contributor's checklist
Here are some reminders before you submit your pull request: