Re: Inaccurate description of UNION/CASE/etc type selection

On Sun, Aug 16, 2020 at 2:26 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:

> We had a question about why an ARRAY[] construct was resolving the
> array's type strangely [1].

In this specific complaint it strikes me as undesirable to have an lossy
implicit cast from text to name - and if that wasn't present then text
would have been chosen as expected.

> The documentation about this [2] says
> that the relevant resolution rules are:
>
> 5. Choose the first non-unknown input type which is a preferred type in
> that category, if there is one.
>
> 6. Otherwise, choose the last non-unknown input type that allows all the
> preceding non-unknown inputs to be implicitly converted to it. (There
> always is such a type, since at least the first type in the list must
> satisfy this condition.)
>
> But what select_common_type() actually does is:
>
> else if (!pispreferred &&
> can_coerce_type(1, &ptype, &ntype, COERCION_IMPLICIT)
> &&
> !can_coerce_type(1, &ntype, &ptype,
> COERCION_IMPLICIT))
> {
> /*
> * take new type if can coerce to it implicitly but not the
> * other way; but if we have a preferred type, stay on it.
> */
> pexpr = nexpr;
> ptype = ntype;
> pcategory = ncategory;
> pispreferred = nispreferred;
> }
>
> (ptype is the currently selected common type, ntype is the next
> input type to consider, and we've already eliminated cases involving
> UNKNOWN.)
>

Seems like 5 and 6 need to be merged - the chosen type is the first one
that all subsequent types can be implicitly cast to. We do not guarantee
that previous types will be implicitly convertible to this type.

In pseudo-code:
else if (can_coerce(n->p)) continue /* matches when pispreferred */
else if (can_coerce(p->n)) "change chosen type"
else continue /* but now we expect a runtime implicit cast not found error
*/

To go along with the above simplification:

/* move on to next one if no new information... */
if (ntype != UNKNOWNOID && ntype != ptype) {
should be written positively as
if (ntype == UNKNOWNOID || ntype == ptype)
continue;

Random thought, once we get to the end of the loop why not conditionally go
over the list a second time so the elements prior to the selected value's
type are re-considered in lieu of the new choice? That at least clears up
the documentation to state that all encountered types are compared against
the chosen type for implicit casting instead of just those appearing after
the one we settled upon. Maybe on the second pass we don't actually change
the chosen type but instead provide for a better error message in the case
of a missing implicit type cast.

I do agree that while this doesn't feel like an ideal algorithm it seems
undesirable to change the implementation - or at least its behavior in
non-error situations. Given the lack of complaints here error handling
improvements don't seem like a win either.

David J.