pgsql: Fix list-munging bug that broke SQL function result coercions. - Mailing list pgsql-committers

From Tom Lane
Subject pgsql: Fix list-munging bug that broke SQL function result coercions.
Date
Msg-id E1kUZyS-0002tb-Qz@gemulon.postgresql.org
Whole thread Raw
List pgsql-committers
Fix list-munging bug that broke SQL function result coercions.

Since commit 913bbd88d, check_sql_fn_retval() can either insert type
coercion steps in-line in the Query that produces the SQL function's
results, or generate a new top-level Query to perform the coercions,
if modifying the Query's output in-place wouldn't be safe.  However,
it appears that the latter case has never actually worked, because
the code tried to inject the new Query back into the query list it was
passed ... which is not the list that will be used for later processing
when we execute the SQL function "normally" (without inlining it).
So we ended up with no coercion happening at run-time, leading to
wrong results or crashes depending on the datatypes involved.

While the regression tests look like they cover this area well enough,
through a huge bit of bad luck all the test cases that exercise the
separate-Query path were checking either inline-able cases (which
accidentally didn't have the bug) or cases that are no-ops at runtime
(e.g., varchar to text), so that the failure to perform the coercion
wasn't obvious.  The fact that the cases that don't work weren't
allowed at all before v13 probably contributed to not noticing the
problem sooner, too.

To fix, get rid of the separate "flat" list of Query nodes and instead
pass the real two-level list that is going to be used later.  I chose
to make the same change in check_sql_fn_statements(), although that has
no actual bug, just so that we don't need that data structure at all.

This is an API change, as evidenced by the adjustments needed to
callers outside functions.c.  That's a bit scary to be doing in a
released branch, but so far as I can tell from a quick search,
there are no outside callers of these functions (and they are
sufficiently specific to our semantics for SQL-language functions that
it's not apparent why any extension would need to call them).  In any
case, v13 already changed the API of check_sql_fn_retval() compared to
prior branches.

Per report from pinker.  Back-patch to v13 where this code came in.

Discussion: https://postgr.es/m/1603050466566-0.post@n3.nabble.com

Branch
------
REL_13_STABLE

Details
-------
https://git.postgresql.org/pg/commitdiff/25378db74fd97f2b10ad44d1f0b2e1f8b0a651f2

Modified Files
--------------
src/backend/catalog/pg_proc.c            |   4 +-
src/backend/executor/functions.c         | 122 +++++++++++++++++--------------
src/backend/optimizer/util/clauses.c     |   7 +-
src/include/executor/functions.h         |   4 +-
src/test/regress/expected/rangefuncs.out |  44 +++++++++++
src/test/regress/sql/rangefuncs.sql      |  11 +++
6 files changed, 132 insertions(+), 60 deletions(-)


pgsql-committers by date:

Previous
From: Heikki Linnakangas
Date:
Subject: pgsql: Misc documentation fixes.
Next
From: Tom Lane
Date:
Subject: pgsql: Fix connection string handling in src/bin/scripts/ programs.