Re: stringtype=unspecified is null check problem

On Wed, Jan 11, 2023 at 8:13 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> "David G. Johnston" <david(dot)g(dot)johnston(at)gmail(dot)com> writes:
> > Yes, the non-determinism of the above (i.e., reversing the order of the
> > tests removes the error), which implies the error is not sufficiently
> > delayed to give other parts of the statement a chance to provide context,
> > is also annoying. Which I suppose is why you are saying a second pass
> > would be needed to get that delay in a minimally-invasive way.
>
> Actually ... looking closer at the relevant code, there already *is*
> two-pass processing. We're just using it to throw an error though.
> I wonder if we can get away with retroactively changing the types
> of Params that didn't get resolved from local context, along the
> lines of the attached. The main issues I can see with this are:
>
> * Is there any case where we'd copy UNKNOWNOID as the type of a
> parse node just above an unresolved Param? I can't think of a
> reason to do that offhand, because any such context would force
> identification of the Param's type; but maybe I'm missing something.
> If that did happen, this code would not fix the type of the upper
> parse node. But maybe that wouldn't matter anyway??
>
> * There is a very gross hack "for JDBC compatibility" right
> adjacent to this:
>
> /*
> * If the argument is of type void and it's procedure call, interpret
> it
> * as unknown. This allows the JDBC driver to not have to distinguish
> * function and procedure calls. See also another component of this
> hack
> * in ParseFuncOrColumn().
> */
> if (*pptype == VOIDOID && pstate->p_expr_kind ==
> EXPR_KIND_CALL_ARGUMENT)
> *pptype = UNKNOWNOID;
>
> I'm not sure if this change breaks that, and have no idea how to test.
>
>
Is the name "check_parameter_resolution_walker" open to change, we are no
longer simply checking here. coerce_unresolved_parameters_hook?

I get the paranoia test shouldn't pass due to testing in
variable_coerce_param_hook, but the errors produced there and here are for
the same condition and probably should match. Or maybe change this one to
an assert?

Re: JDBC, it would have been nice to have tests already in place when the
code had gone in...hopefully the JDBC's project test suite will cover their
six.

Being unable to "PREPARE ... AS CALL proc(...);" does make testing this a
bit trickier. The functional version is just:

create or replace function callfunc(OUT val integer) returns integer as $$
select 1::integer; $$ language sql;
prepare cf (void) as select callfunc($1);
execute cf('text');

Which is still intact. I'm not too concerned here myself. I also did:

create or replace function echo(in val text) returns text as $$ select val;
$$ language sql;
create or replace function echo(in val integer) returns integer as $$
select val; $$ language sql;
prepare cecho (void) as select echo($1);
ERROR: function echo() does not exist
LINE 1: prepare cecho (void) as select echo($1);

A function signature ignores output arguments and missing input arguments
are going to be a problem in their own right. A function call has to
assign concrete output types when it is defined so in theory any function
call arguments are going to be known and the unknown assignment in the hack
already is guaranteed to find a context data type to resolve to so this
failsafe code should never be reached for those void+argument parameters.

If the above reasoning is sound, though, maybe modify the resolvedType test
added to check_parameter_resolution_walker to only consider
non-EXPR_KIND_CALL_ARGUMENT placed parameters.

David J.