Re: MySQL search query is not executing in Postgres DB - Mailing list pgsql-hackers

On Thu, Aug 30, 2012 at 9:13 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> Upthread you were complaining about how we'd reject calls even when
>> there was only one possible interpretation.  I wonder whether there'd be
>> any value in taking that literally: that is, allow use of assignment
>> rules when there is, in fact, exactly one function with the right number
>> of parameters visible in the search path.  This would solve the LPAD()
>> problem (at least as stated), and probably many other practical cases
>> too, since I admit your point that an awful lot of users do not use
>> function overloading.  The max() example I mentioned earlier would not
>> get broken since there's more than one max(), and in general it seems
>> likely that cases where there's a real risk would involve overloaded
>> names.
>
> That's an interesting idea.  I like it.

I did some experimentation with this.  It seems that what Tom proposed
here is a lot cleaner than what I proposed previously, while still
increasing usability in many real-world cases.  For example, in
unpatched master:

rhaas=# create function xyz(smallint) returns smallint as $$select
$1$$ language sql;
CREATE FUNCTION
rhaas=# select xyz(5);
ERROR:  function xyz(integer) does not exist
LINE 1: select xyz(5);
               ^
HINT:  No function matches the given name and argument types. You
might need to add explicit type casts.
rhaas=# create table abc (a int);
CREATE TABLE
rhaas=# select lpad(a, 5, '0') from abc;
ERROR:  function lpad(integer, integer, unknown) does not exist
LINE 1: select lpad(a, 5, '0') from abc;
               ^
HINT:  No function matches the given name and argument types. You
might need to add explicit type casts.

But, with the attached patch:

rhaas=# create function xyz(smallint) returns smallint as $$select
$1$$ language sql;
CREATE FUNCTION
rhaas=# select xyz(5);
 xyz
-----
   5
(1 row)

rhaas=# create table abc (a int);
CREATE TABLE
rhaas=# select lpad(a, 5, '0') from abc;
 lpad
------
(0 rows)

There is only one regression test output change:

-ERROR:  function int2um(integer) does not exist
+ERROR:  function int2um(smallint) requires run-time type coercion

The replacement error message is coming from lookup_agg_function(),
which calls func_get_detail() and then imposes stricter checks on the
result.  In the old coding func_get_detail() didn't even identify a
candidate, whereas now it does but lookup_agg_function() decides that
it isn't usable.  This seems OK to me, and the error message doesn't
seem any worse either.

So that's the good news.  The not-so-good news is that to make it
work, I had to modify make_fn_arguments() to pass COERCION_ASSIGNMENT
rather than COERCION_IMPLICIT to coerce_type().  Otherwise, parsing
succeeds, but then things fall over later when we try to identify the
coercion function to be used.  The reason I'm nervous about is because
the code now looks like this:

                node = coerce_type(pstate,
                                   node,
                                   actual_arg_types[i],
                                   declared_arg_types[i], -1,
                                   COERCION_ASSIGNMENT,
                                   COERCE_IMPLICIT_CAST,
                                   -1);

It seems wrong to pass COERCE_IMPLICIT_CAST along with
COERCION_ASSIGNMENT, because COERCE_IMPLICIT_CAST controls the way
that the cast is *displayed*, and COERCE_IMPLICIT_CAST means don't
display it at all.  That seems like it could create a problem if we
used this new type of argument matching (because there was only one
function with a given name) and then later someone added a second one.
 I thought, for example, that there might be a problem with the way
views are reverse-parsed, but it actually seems to work OK, at least
in the case I can think of to test:

rhaas=# create table look_ma (a int, b text);
CREATE TABLE
rhaas=# create view look_ma_view (a, b) as select lpad(a, 5), lpad(b,
5) from look
CREATE VIEW
rhaas=# \d+ look_ma_view
             View "public.look_ma_view"
 Column | Type | Modifiers | Storage  | Description
--------+------+-----------+----------+-------------
 a      | text |           | extended |
 b      | text |           | extended |
View definition:
 SELECT lpad(look_ma.a::text, 5) AS a, lpad(look_ma.b, 5) AS b
   FROM look_ma;

Note that where the assignment cast was used to find the function to
call, we get a cast in the deparsed query, but in the case where we
used an implicit cast, we don't.  This is exactly as I would have
hoped.  I fear there might be a subtler case where there is an issue,
but so far I haven't been able to find it.  If there in fact is an
issue, I think we can fix it by pushing the logic up from
func_match_argtypes where it is now into func_get_detail;
func_get_detail can then return some indication to the caller
indicating which make_fn_arguments behavior is required.  However, I
don't want to add that complexity unless we actually need it for
something.

Thoughts?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company