Re: 11beta crash/assert caused by parameter type changes - Mailing list pgsql-hackers

Vik Fearing <vik.fearing@2ndquadrant.com> writes:
> On 24/07/18 22:50, Andrew Gierth wrote:
>> Haven't had time yet to poke at what's changed in detail.

> git bisect lays the blame on 6719b238e8f0ea83c39d2b1728c7670cdaa34e06.

Right.  So the reason that this Assert is reachable now is that that
commit made it possible to, as the commit message said, expose the
values of record-field variables to the planner.  All the other places
that call paramFetch hooks either believe the returned "prmtype" or
do run-time checks that it matches what they think it should be.
This one's out of step with a simple assertion, so I think a reasonable
fix would be as attached.

Eventually, of course, we should make such cases Actually Work by forcing
a replan of the cached plan.  But it was not the intent of anything I did
in v11 to make that happen, only to lay a bit more groundwork for it.
So it'd be unreasonable to try to fix that during beta.

I was about to add Andrew's example as a test case (also shown in
attached), but realized that there's a problem: just as noted in
the similar test for named-composite-type changes a bit above there,
the failure fails to fail in CLOBBER_CACHE_ALWAYS builds.  We'd decided
to just omit that test (cf feb1cc559) but having been embarrassed by
this crash I'm feeling like we really could do with some test coverage.
I'm tempted to extract the affected test cases into their own small
test script and provide two variant expected files, one for normal and
one for CLOBBER_CACHE_ALWAYS builds.  Thoughts?

            regards, tom lane

diff --git a/src/backend/optimizer/util/clauses.c b/src/backend/optimizer/util/clauses.c
index 505ae0a..a04ad6e 100644
*** a/src/backend/optimizer/util/clauses.c
--- b/src/backend/optimizer/util/clauses.c
*************** eval_const_expressions_mutator(Node *nod
*** 2567,2573 ****
                      else
                          prm = ¶mLI->params[param->paramid - 1];

!                     if (OidIsValid(prm->ptype))
                      {
                          /* OK to substitute parameter value? */
                          if (context->estimate ||
--- 2567,2580 ----
                      else
                          prm = ¶mLI->params[param->paramid - 1];

!                     /*
!                      * We don't just check OidIsValid, but insist that the
!                      * fetched type match the Param, just in case the hook did
!                      * something unexpected.  No need to throw an error here
!                      * though; leave that for runtime.
!                      */
!                     if (OidIsValid(prm->ptype) &&
!                         prm->ptype == param->paramtype)
                      {
                          /* OK to substitute parameter value? */
                          if (context->estimate ||
*************** eval_const_expressions_mutator(Node *nod
*** 2583,2589 ****
                              bool        typByVal;
                              Datum        pval;

-                             Assert(prm->ptype == param->paramtype);
                              get_typlenbyval(param->paramtype,
                                              &typLen, &typByVal);
                              if (prm->isnull || typByVal)
--- 2590,2595 ----
diff --git a/src/pl/plpgsql/src/expected/plpgsql_record.out b/src/pl/plpgsql/src/expected/plpgsql_record.out
index 6ea88b3..b5129b3 100644
*** a/src/pl/plpgsql/src/expected/plpgsql_record.out
--- b/src/pl/plpgsql/src/expected/plpgsql_record.out
*************** alter table mutable drop column f3;
*** 459,464 ****
--- 459,487 ----
  select getf3(null::mutable);  -- fails again
  ERROR:  record "x" has no field "f3"
  \set SHOW_CONTEXT errors
+ -- check behavior with changes in a record rowtype
+ create function show_result_type(text) returns text language plpgsql as
+ $$
+     declare
+         r record;
+         t text;
+     begin
+         execute $1 into r;
+         select pg_typeof(r.a) into t;
+         return format('type %s value %s', t, r.a::text);
+     end;
+ $$;
+ select show_result_type('select 1 as a');
+    show_result_type
+ ----------------------
+  type integer value 1
+ (1 row)
+
+ -- currently this fails, perhaps someday we can make it succeed
+ select show_result_type('select 2.0 as a');
+ ERROR:  type of parameter 5 (numeric) does not match that when preparing the plan (integer)
+ CONTEXT:  SQL statement "select pg_typeof(r.a)"
+ PL/pgSQL function show_result_type(text) line 7 at SQL statement
  -- check access to system columns in a record variable
  create function sillytrig() returns trigger language plpgsql as
  $$begin
diff --git a/src/pl/plpgsql/src/sql/plpgsql_record.sql b/src/pl/plpgsql/src/sql/plpgsql_record.sql
index aba6887..6f638d6 100644
*** a/src/pl/plpgsql/src/sql/plpgsql_record.sql
--- b/src/pl/plpgsql/src/sql/plpgsql_record.sql
*************** alter table mutable drop column f3;
*** 297,302 ****
--- 297,319 ----
  select getf3(null::mutable);  -- fails again
  \set SHOW_CONTEXT errors

+ -- check behavior with changes in a record rowtype
+ create function show_result_type(text) returns text language plpgsql as
+ $$
+     declare
+         r record;
+         t text;
+     begin
+         execute $1 into r;
+         select pg_typeof(r.a) into t;
+         return format('type %s value %s', t, r.a::text);
+     end;
+ $$;
+
+ select show_result_type('select 1 as a');
+ -- currently this fails, perhaps someday we can make it succeed
+ select show_result_type('select 2.0 as a');
+
  -- check access to system columns in a record variable

  create function sillytrig() returns trigger language plpgsql as