Moving discussion to -hackers On Sun, May 1, 2022 at 12:46 PM Tom Lane <t...@sss.pgh.pa.us> wrote:
> "David G. Johnston" <david.g.johns...@gmail.com> writes:
> > On Sun, May 1, 2022 at 10:08 AM Tom Lane <t...@sss.pgh.pa.us> wrote:
> >> Maybe we could improve this situation by treating a "record" parameter
> >> as polymorphic, though that might cause some odd inconsistencies with
> >> plpgsql's historical treatment of "record" local variables.
>
> > The extent of needing to treat "record" as polymorphic-like seems like it
> > would be limited to resolve_polymorphic_argtype in funcapi.c. Namely, in
> > computing the hash key for the compiled hash entry for the function.
> > Similar to how we append the trigger oid in compute_function_hashkey in
> > pl.compile (which ultimately calls the former) so trigger invocations
> > become per-table.
>
> I'm hesitant to touch funcapi.c for this; the scope of potential
> side-effects becomes enormous as soon as you do.
>
>
Agreed, though the only caller of this particular function seems to be in
plpgsql/pl_comp.c anyway...one more-or-less directly from do_compile and
the other via compute_function_hashkey, so its presence in funcapi.c seems
unusual.
But, to get rid of the cache error it seems sufficient to simply ensure we
compute a hash key that considers the called types.
That do_compile doesn't see these substitutions is a positive for minimal
potential impact with no apparent downside.
I added the test case from the -bug email (I'd probably change it to match
the "scenario" for the file for a final version).
I have some questions in the code that I could probably get answers for
via tests - would including those tests be acceptable (defaults,
named-argument-calling, output argmode)?
I did check-world without issue - but I suspect this to be under-tested and
not naturally used in the course of unrelated testing.
David J.
+ /*
+ * Saved compiled functions with record-typed input args to a
hashkey
+ * that substitutes all known actual composite type OIDs in the
+ * call signature for the corresponding generic record OID from
+ * the definition signature. This avoids a probable error:
+ * "type of parameter ... does not match that when preparing the
plan"
+ * when such a record variable is used in a query within the
function.
+ */
+ for (int i = 0; i < procStruct->pronargs; i++)
+ {
+ if (hashkey->argtypes[i] != RECORDOID)
+ continue;
+
+ // ??? I presume other parts of the system synchronize these
two arrays
+ // In particular for default arguments not specified in the
function call
+ // or named-argument function call syntax.
+
+ /* Don't bother trying to substitute once we run out of input
arguments */
+ if (i > fcinfo->nargs - 1)
+ break;
+
+ hashkey->argtypes[i] =
+ get_call_expr_argtype(fcinfo->flinfo->fn_expr, i);
+ }
+select record_to_form_data(fruit1) from fruit1;
+ record_to_form_data
+---------------------
+ (1,apple,red)
+(1 row)
+
+select record_to_form_data(fruit2) from fruit2;
+ record_to_form_data
+---------------------
+ (1,apple)
+(1 row)
+
v0001-cache-record-input-function-based-on-call-arguments.patch
Description: Binary data
