[PATCH] regression test: view and rule definitions must round trip
Hackorum builds and tests every patch posted to the lists, not only commitfest submissions. This is Hackorum's own CI rather than the PostgreSQL project's, and it is still under testing - please report anything that looks wrong.
You can run a PostgreSQL built from this patch straight from Docker, with no checkout and no build:
docker run --rm -p 5432:5432 ghcr.io/hackorum-dev/postgres-patch:t254395psql -h localhost -U postgresBuilt from patchset v1 (message #1), October 26, 2026 at 02:24 PM.
Every patchset is also pushed to a branch of our PostgreSQL fork, so you can check out the same tree CI built. Without a PostgreSQL checkout:
git clone --branch t254395_1 https://github.com/hackorum-dev/postgres.gitIn a checkout you already have, add the fork once:
git remote add hackorum https://github.com/hackorum-dev/postgres.gitthen, for this patchset and every later one:
git fetch hackorum t254395_1 && git checkout t254395_1Patchset v1 (message #1) is on t254395_1
Hello hackers,
PostgreSQL can show you the definition of a view with pg_get_viewdef()
(and a rule with pg_get_ruledef()). This is how pg_dump writes views
into a backup. The rule that has to hold: the text it gives back must be
valid SQL that recreates the same view. If it hands you SQL that won't
parse, your backup won't restore. My test checks that this holds for a
batch of tricky view and rule definitions, so a future code change can't
quietly break it.
But this is fragile because when you write a view, PostgreSQL doesn't
store your original SQL text, it stores a parsed tree, then reconstructs
the SQL text from that tree whenever you ask for the definition. That
reconstruction step (the "deparser") has to get the spelling exactly
right (every parenthesis, every quote, every keyword). It mostly does,
but there are corners where it drops something, and then the
reconstructed text is subtly invalid.
I was working with a differential selectivity test framework and I ran
into the three real examples (all shipped as bugs at some point):
- A COLLATE clause inside an XMLNAMESPACES URI came back without
parentheses, and the grammar doesn't allow a bare COLLATE there — so
the recreated view failed to parse with "syntax error at or near
COLLATE".
- Operators on ROW() constructors could read back wrong.
- A rule whose action uses a table alias named "set" (a reserved-ish
word) came back unquoted as UPDATE t set SET ..., which is a syntax
error.
The common thread: the reconstruction is a separate piece of code from
the parser, so the two can drift, and it is very hard to notice until a
specific weird definition trips it.
Using the framework I've been developing I was able to create a series
of teststhat try to codify this contract so as to be more noticable in
the future if/when some change overlooks this requirement. Essentially,
for each view in the corpus it does a round-trip and asserts three
things:
1. It re-parses: take the definition pg_get_viewdef() hands back, feed
it to CREATE VIEW again; it must succeed (not a syntax error).
2. Same rows: the recreated view must return the same data as the
original (the text didn't just parse, it means the same thing).
3. Fixed point: deparse the recreated view and compare to the deparse
of the original; they must be byte-identical (reconstructing it a second
time doesn't drift further).
Each case prints just ok or FAIL <why>, so the expected-output file
stays small and doesn't care how the deparser chooses to spell a given
view — only that the round-trip holds.
There are round-trip-ish checks scattered through rules.sql,
create_view.sql, xml.sql but they work by someone adding a tricky view,
eyeballing that \d+ looks right, and moving on. AFAICT, there's no
single test that says "take this whole corpus of known-nasty shapes and
assert the round-trip property for every one." That gap is exactly why
the XMLNAMESPACES bug and its three siblings shipped: no systematic
guard caught them. The test intends to fills that gap with one generic
oracle instead of dozens of ad-hoc eyeball checks.
What's in the patch?
Put simply, it is a way to quickly codify and test for this kind of
issue in a single location in our regression tests. The 17 cases I've
added are expressions needing parentheses, ROW() comparisons, CASE,
DISTINCT ON, set operations, grouping sets, keyword-named windows,
window frames, IS NOT DISTINCT FROM, array subscripts, casts, correlated
subqueries, plus a few rules and they all pass (as espected) today but
importantly they start to close this gap permanently.
The attached patch adds src/test/regress/sql/deparse_roundtrip.sql. It
drives a corpus of view and rule definitions through two plpgsql
helpers: rt_view creates the view, deparses it, recreates the deparsed
text, and checks it re-parsed, returns the same rows, and is a deparse
fixed point; rt_rule does the re-parse check for a rule action. Each
case reports only ok or FAIL so the expected output stays small and does
not depend on how the deparser spells a given definition.
The test is self-contained (one schema, dropped at the end), pure SQL,
no new dependency. Added to the create_view parallel group.
best.
-greg
On Mon, Oct 26, 2026 at 3:13 PM Greg Burd <greg@burd.me> wrote:
Hello hackers,
PostgreSQL can show you the definition of a view with pg_get_viewdef()
(and a rule with pg_get_ruledef()). This is how pg_dump writes views
into a backup. The rule that has to hold: the text it gives back must be
valid SQL that recreates the same view. If it hands you SQL that won't
parse, your backup won't restore. My test checks that this holds for a
batch of tricky view and rule definitions, so a future code change can't
quietly break it.But this is fragile because when you write a view, PostgreSQL doesn't
store your original SQL text, it stores a parsed tree, then reconstructs
the SQL text from that tree whenever you ask for the definition. That
reconstruction step (the "deparser") has to get the spelling exactly
right (every parenthesis, every quote, every keyword). It mostly does,
but there are corners where it drops something, and then the
reconstructed text is subtly invalid.I was working with a differential selectivity test framework and I ran
into the three real examples (all shipped as bugs at some point):- A COLLATE clause inside an XMLNAMESPACES URI came back without
parentheses, and the grammar doesn't allow a bare COLLATE there — so
the recreated view failed to parse with "syntax error at or near
COLLATE".- Operators on ROW() constructors could read back wrong.
- A rule whose action uses a table alias named "set" (a reserved-ish
word) came back unquoted as UPDATE t set SET ..., which is a syntax
error.The common thread: the reconstruction is a separate piece of code from
the parser, so the two can drift, and it is very hard to notice until a
specific weird definition trips it.Using the framework I've been developing I was able to create a series
of teststhat try to codify this contract so as to be more noticable in
the future if/when some change overlooks this requirement. Essentially,
for each view in the corpus it does a round-trip and asserts three
things:1. It re-parses: take the definition pg_get_viewdef() hands back, feed
it to CREATE VIEW again; it must succeed (not a syntax error).2. Same rows: the recreated view must return the same data as the
original (the text didn't just parse, it means the same thing).3. Fixed point: deparse the recreated view and compare to the deparse
of the original; they must be byte-identical (reconstructing it a second
time doesn't drift further).Each case prints just ok or FAIL <why>, so the expected-output file
stays small and doesn't care how the deparser chooses to spell a given
view — only that the round-trip holds.There are round-trip-ish checks scattered through rules.sql,
create_view.sql, xml.sql but they work by someone adding a tricky view,
eyeballing that \d+ looks right, and moving on. AFAICT, there's no
single test that says "take this whole corpus of known-nasty shapes and
assert the round-trip property for every one." That gap is exactly why
the XMLNAMESPACES bug and its three siblings shipped: no systematic
guard caught them. The test intends to fills that gap with one generic
oracle instead of dozens of ad-hoc eyeball checks.
If those views are left behind by the test in the regression database
002_pg_upgrade will dump those views, restore them, dump them again
and compare the original dump and restored dump. That should cover the
round trip you are trying to test. Are the views testing the features
above left behind by the testcase or dropped?
--
Best Wishes,
Ashutosh Bapat
On Monday, October 26th, 2026 at 2:36 PM, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
On Mon, Oct 26, 2026 at 3:13 PM Greg Burd <greg@burd.me> wrote:
Hello hackers,
PostgreSQL can show you the definition of a view with pg_get_viewdef()
(and a rule with pg_get_ruledef()). This is how pg_dump writes views
into a backup. The rule that has to hold: the text it gives back must be
valid SQL that recreates the same view. If it hands you SQL that won't
parse, your backup won't restore. My test checks that this holds for a
batch of tricky view and rule definitions, so a future code change can't
quietly break it.But this is fragile because when you write a view, PostgreSQL doesn't
store your original SQL text, it stores a parsed tree, then reconstructs
the SQL text from that tree whenever you ask for the definition. That
reconstruction step (the "deparser") has to get the spelling exactly
right (every parenthesis, every quote, every keyword). It mostly does,
but there are corners where it drops something, and then the
reconstructed text is subtly invalid.I was working with a differential selectivity test framework and I ran
into the three real examples (all shipped as bugs at some point):- A COLLATE clause inside an XMLNAMESPACES URI came back without
parentheses, and the grammar doesn't allow a bare COLLATE there — so
the recreated view failed to parse with "syntax error at or near
COLLATE".- Operators on ROW() constructors could read back wrong.
- A rule whose action uses a table alias named "set" (a reserved-ish
word) came back unquoted as UPDATE t set SET ..., which is a syntax
error.The common thread: the reconstruction is a separate piece of code from
the parser, so the two can drift, and it is very hard to notice until a
specific weird definition trips it.Using the framework I've been developing I was able to create a series
of teststhat try to codify this contract so as to be more noticable in
the future if/when some change overlooks this requirement. Essentially,
for each view in the corpus it does a round-trip and asserts three
things:1. It re-parses: take the definition pg_get_viewdef() hands back, feed
it to CREATE VIEW again; it must succeed (not a syntax error).2. Same rows: the recreated view must return the same data as the
original (the text didn't just parse, it means the same thing).3. Fixed point: deparse the recreated view and compare to the deparse
of the original; they must be byte-identical (reconstructing it a second
time doesn't drift further).Each case prints just ok or FAIL <why>, so the expected-output file
stays small and doesn't care how the deparser chooses to spell a given
view — only that the round-trip holds.There are round-trip-ish checks scattered through rules.sql,
create_view.sql, xml.sql but they work by someone adding a tricky view,
eyeballing that \d+ looks right, and moving on. AFAICT, there's no
single test that says "take this whole corpus of known-nasty shapes and
assert the round-trip property for every one." That gap is exactly why
the XMLNAMESPACES bug and its three siblings shipped: no systematic
guard caught them. The test intends to fills that gap with one generic
oracle instead of dozens of ad-hoc eyeball checks.If those views are left behind by the test in the regression database
002_pg_upgrade will dump those views, restore them, dump them again
and compare the original dump and restored dump. That should cover the
round trip you are trying to test.
I'm not sure what you're suggesting, yes that is true but does that mean
that you'd prefer that method for validation over what I've proposed?
Are the views testing the features above left behind by the testcase
or dropped?
They are in a schema and removed at the end of the test.
--
Best Wishes,
Ashutosh Bapat
The purpose of the suggested patch is to simply how we codify and ensure
that this kind of bug isn't re-introduced down the road.
best.
-greg
On Mon, Oct 26, 2026 at 7:43 PM Greg Burd <greg@burd.me> wrote:
AFAICT, there's no
single test that says "take this whole corpus of known-nasty shapes and
assert the round-trip property for every one."
... snip ...
I'm not sure what you're suggesting, yes that is true but does that mean
that you'd prefer that method for validation over what I've proposed?
I am simply pointing out that a test which you said doesn't exist
above. I would rather use that infrastructure (which works for objects
other than views as well) that creating a new one specifically for
views. If we could build a better way to do this for all objects, that
would be great. That possibility has been discussed earlier.
Are the views testing the features above left behind by the testcase
or dropped?They are in a schema and removed at the end of the test.
That explains why we missed catching those bugs when they were added.
--
Best Wishes,
Ashutosh Bapat
Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
On Mon, Oct 26, 2026 at 7:43 PM Greg Burd <greg@burd.me> wrote:
I'm not sure what you're suggesting, yes that is true but does that mean
that you'd prefer that method for validation over what I've proposed?
I am simply pointing out that a test which you said doesn't exist
above. I would rather use that infrastructure (which works for objects
other than views as well) that creating a new one specifically for
views. If we could build a better way to do this for all objects, that
would be great. That possibility has been discussed earlier.
I share Ashutosh's doubts about whether this is a better way than the
existing pg_dump round-trip test. It is cute that you try to actually
execute the re-parsed view and see if the results are the same; the
existing methodology does nothing in that direction. But with such
necessarily-restricted input data I'm not sure how much that proves
really.
To my mind the biggest problems we've had with ruleutils bugs are
(1) Failure to think about, or to test, dumping of new syntax at all.
Unfortunately this approach doesn't help that, because if you've
overlooked the issue then you won't think to add a new test case
here either.
(2) Unexpected interactions between syntax features, such as the
parenthesization bugs that Henson Choi is working through on
nearby threads. I don't see that this approach moves us very far
on that problem either, because it does little about combining
different features. A brute force approach to that would be O(N^2)
or worse, so I'm not sure how much can be done automatically.
More generally, I'm not really convinced that I like this black-box
approach to testing ruleutils behavior. In places such as rules.sql's
related tests, we're interested in whether the output is stable
across branches and looks reasonable to a human; neither thing can
be determined with this test, since it intentionally hides the
deparsed output.
regards, tom lane
On Monday, October 26th, 2026 at 3:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> writes:
On Mon, Oct 26, 2026 at 7:43 PM Greg Burd <greg@burd.me> wrote:
I'm not sure what you're suggesting, yes that is true but does that mean
that you'd prefer that method for validation over what I've proposed?I am simply pointing out that a test which you said doesn't exist
above. I would rather use that infrastructure (which works for objects
other than views as well) that creating a new one specifically for
views. If we could build a better way to do this for all objects, that
would be great. That possibility has been discussed earlier.I share Ashutosh's doubts about whether this is a better way than the
existing pg_dump round-trip test. It is cute that you try to actually
execute the re-parsed view and see if the results are the same; the
existing methodology does nothing in that direction. But with such
necessarily-restricted input data I'm not sure how much that proves
really.
Fair points. I see the objection clearly now, thanks for writing it up.
I saw a repeating theme and took a stab at creating a way to capture it.
Seems that it was miss.
To my mind the biggest problems we've had with ruleutils bugs are
(1) Failure to think about, or to test, dumping of new syntax at all.
Unfortunately this approach doesn't help that, because if you've
overlooked the issue then you won't think to add a new test case
here either.
Yes, on this we agree. Somehow I'd imagined that having something
simple and central would add some awareness of the issue earlier in the
process.
(2) Unexpected interactions between syntax features, such as the
parenthesization bugs that Henson Choi is working through on
nearby threads. I don't see that this approach moves us very far
on that problem either, because it does little about combining
different features. A brute force approach to that would be O(N^2)
or worse, so I'm not sure how much can be done automatically.
Yes, I've seen his good work in that regard.
More generally, I'm not really convinced that I like this black-box
approach to testing ruleutils behavior. In places such as rules.sql's
related tests, we're interested in whether the output is stable
across branches and looks reasonable to a human; neither thing can
be determined with this test, since it intentionally hides the
deparsed output.
I differ here in that I do like a clean wrapped interface that
encapsulates a kind of test making it a) easier to contribute, b) more
prominent as a failure type that we should cover, and c) less verbose.
I like the simple worked/didn't work style of this, but that's just me.
regards, tom lane
It sounds like I should drop this idea, and focus on other things and/or
helping Choi in his efforts. And that's a good outcome too. :)
best.
-greg