Hi jian,
jian he <[email protected]> wrote:
> I choose to disallow UNKNOWN target types in find_coercion_pathway.
I reviewed v2 on master at 9e17d25e79d. It applies cleanly, builds
without warnings, and make check passes. The new test in misc fails
without the parse_coerce.c change (I applied only the test files to
master), so it does exercise the fix.
It does what Tom asked for: statements that reached the internal
"failed to find conversion function" elog now get a user-level error,
42846 instead of XX000. One problem, though. Besides the PL/pgSQL
assignment you mention, v2 rejects statements that work today, and
some of them can be stored. On master all of these succeed:
create table t (a text);
create view v1 as select cast(a as unknown)::text as c from t;
create view v2 as select cast(a as unknown) is null as c from t;
create index i on t ((cast(a as unknown)::text));
create table t_chk (a text check (cast(a as unknown) is not null));
create table t_def (a text default cast('x'::text as unknown)::text);
create function f() returns text
begin atomic select cast('x'::text as unknown)::text; end;
A pg_dump of that database restored onto v2 fails for all six objects
with "cannot cast type text to unknown". Since the CHECK and the
DEFAULT are part of CREATE TABLE, t_chk and t_def are missing after the
restore, and so is their data. pg_upgrade from such a cluster to v2
fails as well:
pg_restore: error: could not execute query: ERROR: cannot cast
type text to unknown
Casting to unknown on purpose is unlikely, but when it is there the
cost is a failed upgrade.
The same elog was item 6 in Alexander's "Internal error codes triggered
by regression tests and user queries, take 2" [1], and it was left
aside because it was only reachable through unknownin(), an internal
input function. This thread shows it is also reachable with plain SQL:
a CAST to unknown, which is listed among the pseudo-types in the docs,
and from there through CREATE VIEW, CREATE TABLE AS and PREPARE.
So there is a narrower alternative to v2. Literals are handled at the
top of coerce_type(), so the only way to reach the "caller blew it"
elog from SQL is with an unknown input that is not a literal. Raising
ERRCODE_CANNOT_COERCE for that case only keeps the elog for real caller
bugs, and it can reuse "cannot cast type %s to %s", which parse_coerce.c
already uses four times and is translated. I tried it (attached as
.txt so the CF bot does not pick it up): the twelve statements in my
tests that give XX000 on master all get 42846, the statements that work
today keep working, the dump above restores with no errors, pg_upgrade
succeeds with the data in place, and make check passes.
The behavioral difference is that the cast itself stays legal and the
error is raised where the value is used:
master: failed to find conversion function from unknown to text
v2: cannot cast type text to unknown
narrower: cannot cast type unknown to text
If rejecting the cast outright is preferred, as v2 does, it would need
an answer for expressions that are already stored. I don't have a
strong opinion on which way is better, but these numbers seemed worth
having before choosing.
Two small things in v2: the new test lands at the end of misc.sql under
the "-- rewrite rules" heading, which it is unrelated to, and the
"commitfest entry:" line in the commit message is empty.
The SQL I used for the comparison is attached as well.
[1] https://www.postgresql.org/message-id/apUcBVyazHEW5pcg%40paquier.xyz
Regards,
Manu
diff --git a/src/backend/parser/parse_coerce.c
b/src/backend/parser/parse_coerce.c
index d3240f4b265..83ed74a22a4 100644
--- a/src/backend/parser/parse_coerce.c
+++ b/src/backend/parser/parse_coerce.c
@@ -540,6 +540,22 @@ coerce_type(ParseState *pstate, Node *node,
r->location = location;
return (Node *) r;
}
+
+ /*
+ * Callers assume that any input of type unknown is a literal, which can
+ * be coerced to anything (see can_coerce_type()), and literals were
+ * handled above. An unknown-typed expression that is not a literal,
such
+ * as an explicit cast to unknown or a call to unknownin(), gets here,
so
+ * report that as a user error rather than an internal one.
+ */
+ if (inputTypeId == UNKNOWNOID)
+ ereport(ERROR,
+ (errcode(ERRCODE_CANNOT_COERCE),
+ errmsg("cannot cast type %s to %s",
+ format_type_be(inputTypeId),
+ format_type_be(targetTypeId)),
+ parser_coercion_errposition(pstate, location,
node)));
+
/* If we get here, caller blew it */
elog(ERROR, "failed to find conversion function from %s to %s",
format_type_be(inputTypeId), format_type_be(targetTypeId));
-- #7094 matriz master vs v2: cada sentencia imprime OK o ERROR + SQLSTATE.
-- XX000 = elog interno (no deberia ser alcanzable desde SQL);
-- 42846 = cannot_coerce (error de usuario).
\set ON_ERROR_STOP off
\set SHOW_CONTEXT never
\pset footer off
\echo '=== A. el bug reportado'
\echo '--- A1 select cast(NULL::text as unknown)'
select cast(NULL::text as unknown);
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- A2 cast de una columna text a unknown'
create table t(a text); insert into t values ('x');
select cast(a as unknown) from t;
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '=== B. contextos donde el unknown NO se resuelve a text (master podria
funcionar hoy)'
\echo '--- B1 ... is null'
select cast('a'::text as unknown) is null;
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- B2 pg_typeof(...)'
select pg_typeof(cast('a'::text as unknown));
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- B3 length(...)'
select length(cast('a'::text as unknown));
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- B4 where ... = literal'
select 1 as uno where cast('a'::text as unknown) = 'a';
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- B5 values(...)'
select count(*) from (values (cast('a'::text as unknown))) v(x);
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- B6 (...)::text'
select cast('a'::text as unknown)::text;
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '=== C. PL/pgSQL (jian: HEAD acepta la asignacion via I/O)'
\echo '--- C1 int := text::unknown'
do $$ declare a int; begin a := '1'::text::unknown; raise notice 'a=%', a; end
$$;
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- C2 text := text::unknown (el caso del test del patch)'
do $$ declare a text; begin a := 's'::text::unknown; raise notice 'a=%', a; end
$$;
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- C3 text := literal::unknown (control)'
do $$ declare a text; begin a := 'x'::unknown; raise notice 'a=%', a; end $$;
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '=== D. controles que NO deberian cambiar'
\echo '--- D1 literal::unknown'
select 'a'::unknown;
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- D2 cast(literal as unknown)'
select cast('a' as unknown);
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- D3 cast(1 as unknown)'
select cast(1 as unknown);
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- D4 literal::unknown::text'
select 'a'::unknown::text;
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- D5 literal::unknown + 1'
select '1'::unknown + 1;
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '=== E. cast con sintaxis de funcion (parse_func.c llama a
find_coercion_pathway)'
\echo '--- E1 unknown(literal)'
select unknown('a');
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- E2 unknown(text)'
select unknown('a'::text);
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- E3 unknown(unknownin(...)) -- unknown -> unknown sin ser Const'
select unknown(unknownin('a'));
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- E4 pg_typeof(unknownin(...)) (control: unknownin devuelve unknown)'
select pg_typeof(unknownin('a'));
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '=== F. arrays'
\echo '--- F1 existe _unknown?'
select typname, typarray from pg_type where typname in ('unknown', '_unknown')
order by 1;
\echo '--- F2 text[]::unknown[]'
select '{a}'::text[]::unknown[];
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '=== G. objetos que se guardan'
\echo '--- G1 create view ... cast(text as unknown)'
create view v1 as select cast('a'::text as unknown) as c;
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- G2 create table as ... cast(text as unknown)'
create table ctas as select cast('a'::text as unknown) as c;
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '=== U. la OTRA puerta: unknownin() tambien produce un unknown que no es
literal'
\echo '--- U1 select unknownin(a)'
select unknownin('a');
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- U2 select length(unknownin(a))'
select length(unknownin('a'));
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '--- U3 select unknownin(a)::text'
select unknownin('a')::text;
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
\echo '=== H. prepared'
\echo '--- H1 prepare p(text) as select $1::unknown'
prepare p1(text) as select $1::unknown;
\if :ERROR \echo ' => ERROR' :LAST_ERROR_SQLSTATE \else \echo ' => OK'
\endif
