Schema-qualify the equality operator when deparsing NULLIF/IS DISTINCT FROM

michal.dtz@gmail.com writes:

A view that applies NULLIF or IS [NOT] DISTINCT FROM to a value whose
"=" operator is not on the search_path (for example an hstore column)
cannot be dumped and restored.

Yeah, this has been a known issue for a long time. I cataloged a
bunch of related cases at

/messages/by-id/10492.1531515255@sss.pgh.pa.us

but I missed JOIN USING, which also fails to mention exactly which
operator it resolved the semantics with. It doesn't seem hugely
helpful to fix one case without fixing them all, and fixing them all
is a lot of work :-(

Proposed fix (in the deparser)
------------------------------
When the equality operator would not be found by its bare name under the
current search_path -- detected via generate_operator_name(), which
already decides when the OPERATOR(...) decoration is required -- emit an
equivalent expression that can carry the qualified operator, instead of
the normal syntax:

   NULLIF(a, b)
       -> CASE WHEN a IS NOT NULL AND b IS NOT NULL AND (a OPERATOR(s.=) b)
               THEN NULL ELSE a END

Don't like this approach one bit. While the output it produces might
be semantically equivalent (for non-volatile expressions anyway),
it's not equivalent performance-wise, especially not if the change
blocks any optimizations. Also, other cases such as JOIN USING really
can't be fixed without new syntax.

I'm kind of surprised that we haven't gotten more complaints since
2018, but there really haven't been all that many, so we never got
to the point of putting in the work to fix this topic properly.
If you feel motivated, though, have at it.

regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> writes:

Don't like this approach one bit.  While the output it produces might
be semantically equivalent (for non-volatile expressions anyway),
it's not equivalent performance-wise, especially not if the change
blocks any optimizations.  Also, other cases such as JOIN USING really
can't be fixed without new syntax.

Thanks for the review.  You are right that rewriting these constructs
into explicit-operator expressions at deparse time was a dead end: the
rewritten forms evaluate their inputs more than once and stop being
the construct the user wrote.  v1 is withdrawn.

Attached is v2, which takes the opposite approach: instead of teaching
the deparser to avoid the constructs, it gives every construct that
resolves an operator by unqualified name a place to write a
schema-qualified one.  That covers the full list you cataloged back in
2018 (10492.1531515255@sss.pgh.pa.us) -- IS [NOT] DISTINCT FROM,
NULLIF, simple CASE, row comparisons, ANY/ALL subquery comparisons --
plus JOIN USING / NATURAL JOIN, which has the same disease.

Why bother?  Trawling the lists turned up roughly eleven independent
field reports of this failure class since 2014.  IS DISTINCT FROM
accounts for seven of them, mostly via trigger WHEN clauses comparing
extension types (citext, hstore, PostGIS), where the dump either fails
to restore or the trigger definition changes meaning.  JOIN USING
appears twice, including the 2020 case of a citext join inside a
materialized view that silently returned wrong results after
dump/reload
(CAC35HNnNGavaZ=P=rUcwTwYEhfoyXDg32REXCRDgxBmC3No3nA@mail.gmail.com).
NULLIF appears twice.  And while writing the tests I
found one more that nobody had reported: a multi-column
(a, b) IN (SELECT ...) whose columns resolve equality operators from
different schemas silently swaps one column's operator on reload
*today* -- ruleutils prints only the first column's operator name and
even has a comment admitting the approximation.  Patch 0004 carries a
regression test whose setup, run on unpatched master, reproduces the
corruption.

The design is one principle applied six times.  The parser always
accepts an OPERATOR() decoration naming the operator(s) explicitly;
ruleutils.c emits the decoration only when reparsing the undecorated
form would not resolve the very same operator OIDs -- that is, when
the operator is not reachable by its bare name under the prevailing
search path, or is not named "=" where "=" is what an undecorated
reparse would look up.  Dumps of ordinary databases are byte-for-byte
unchanged.  Parse analysis produces exactly the same expression trees
as before: v2 changes only what ruleutils emits -- the executor node
trees are untouched, so plan shapes, the new PG 19 IS DISTINCT FROM
simplifications, and evaluation counts are all unaffected, and no
planner optimization is blocked.

The syntax, per construct:

   IS [NOT] DISTINCT FROM   a IS DISTINCT OPERATOR(s.=) FROM b
   NULLIF                   NULLIF(a, b USING OPERATOR(s.=))
   JOIN USING               JOIN t USING (a, b) OPERATOR (s.=, s.=)
   row comparison           ROW(a, b) OPERATOR(s.<, s.<) ROW(c, d)
   ANY/ALL subquery         (a, b) OPERATOR(s.=, s.=) ANY (SELECT ...)
   simple CASE              CASE x WHEN y USING OPERATOR(s.=) THEN ...

No new keywords; the grammar reuses the existing OPERATOR() production
and extends it to carry a comma-separated list where a construct
compares multiple columns.  bison 3.8.2 reports zero shift/reduce or
reduce/reduce conflicts (checked with counterexample analysis at every
step, since several superficially nicer spellings do conflict), and
the regression tests exercise the forms in combination: mixed
bare/qualified lists, arity mismatches, non-boolean operators, and
b_expr contexts.

The patches, bisectable (the core regression suite is green at every
step of the series; check-world at the tip):

 0001  IS [NOT] DISTINCT FROM and NULLIF, plus a contrib/hstore
       round-trip test against a real extension "=" and a trigger
       WHEN-clause round-trip test.
       (12 files changed, 658 insertions(+), 10 deletions(-))
 0002  JOIN USING and NATURAL JOIN; JoinExpr gains a usingOperators
       field, hence a catversion bump.
       (9 files changed, 372 insertions(+), 29 deletions(-))
 0003  Row comparisons; the single-name OPERATOR() decoration
       generalizes to a per-column list.  Contexts that accept an
       operator name but not a list (CREATE OPERATOR, DDL options)
       reject lists with a uniform error message.
       (12 files changed, 532 insertions(+), 27 deletions(-))
 0004  ANY/ALL and row-op-subquery sublinks; fixes the silent IN
       operator swap described above.  Raw-parse-only change, no
       catversion bump.
       (7 files changed, 524 insertions(+), 40 deletions(-))
 0005  Simple CASE, one optional USING OPERATOR() per WHEN arm;
       CaseWhen gains a field, hence a catversion bump.
       (9 files changed, 304 insertions(+), 8 deletions(-))

The catversion bumps in 0002 and 0005 are included so that testers get
a clean initdb; whoever commits this will of course re-bump.

Some spellings are open to bikeshedding, and I am happy to rework
them.  In particular NULLIF could take the operator as a third
argument, NULLIF(a, b, OPERATOR(s.=)), and JOIN USING could attach
operators per column, USING (a OPERATOR(s.=), b), instead of the
trailing list; I validated both alternatives as conflict-free before
settling on the attached forms, which keep the column list readable
and match the row-comparison list style.  Likewise, a row comparison
whose columns all use the same qualified operator currently prints the
full repeated list rather than a single-element decoration; that is
deliberate (explicit is better than clever) but easily changed.

Full disclosure: this series was developed with substantial AI
assistance (Claude).  Every grammar candidate was validated with
bison before adoption, every patch went through adversarial review,
and the full check-world plus the pg_upgrade dump/restore round-trip
of the regression database (which keeps all the new views, and a
trigger whose WHEN clause exercises the same deparse path, precisely so
that 002_pg_upgrade re-parses them) are green locally on macOS.

If the direction looks right I will register this in the next
commitfest (PG 20-1).  Feedback on the syntax choices is especially
welcome for the simple CASE arm clause and the row-comparison operator
lists, where the SQL committee gives us the least precedent to lean
on.

Best regards,
Michał Pasternak

--
Z uszanowaniem, Michał Pasternak
📱+48793668733
📧 m@iplweb.pl https://bpp.iplweb.pl/
🗓️ https://calendly.com/mpasternak/

michal.dtz@gmail.com writes:

Attached is v2, which takes the opposite approach: instead of teaching
the deparser to avoid the constructs, it gives every construct that
resolves an operator by unqualified name a place to write a
schema-qualified one. That covers the full list you cataloged back in
2018 (10492.1531515255@sss.pgh.pa.us) -- IS [NOT] DISTINCT FROM,
NULLIF, simple CASE, row comparisons, ANY/ALL subquery comparisons --
plus JOIN USING / NATURAL JOIN, which has the same disease.

I didn't read the patch yet, but this sounds promising.

If the direction looks right I will register this in the next
commitfest (PG 20-1).

Next one is already 20-2, but please do register it.

regards, tom lane