I wrote:
> After thinking a bit more about the existing special case for non-foreign
> Vars, I wonder if what we should do is change these code blocks to look
> like

>                 collation = r->resultcollid;
>                 if (collation == InvalidOid)
>                     state = FDW_COLLATE_NONE;
>                 else if (inner_cxt.state == FDW_COLLATE_SAFE &&
>                          collation == inner_cxt.collation)
>                     state = FDW_COLLATE_SAFE;
> +             else if (collation == DEFAULT_COLLATION_OID)
> +                 state = FDW_COLLATE_NONE;
>                 else
>                     state = FDW_COLLATE_UNSAFE;

On further thought, maybe it's the special case for non-foreign Vars
that is busted.  That is, non-foreign Vars should do

                    /* Var belongs to some other table */
                    if (var->varcollid != InvalidOid &&
                        var->varcollid != DEFAULT_COLLATION_OID)
                        return false;

-                   /* We can consider that it doesn't set collation */
-                   collation = InvalidOid;
-                   state = FDW_COLLATE_NONE;
+                   collation = var->varcollid;
+                   state = OidIsValid(collation) ? FDW_COLLATE_SAFE : 

and likewise for Consts, Params, etc.

This would basically mean clarifying the meaning of the state values as:

FDW_COLLATE_NONE: the expression is of a noncollatable type, period.

FDW_COLLATE_SAFE: the expression has default collation, or a nondefault
collation that is traceable to a foreign Var.

FDW_COLLATE_UNSAFE: the expression has a nondefault collation that is not
traceable to a foreign Var.

Hm ... actually, we probably need *both* types of changes if that's
what we believe the state values mean.

An alternative definition would be that FDW_COLLATE_NONE subsumes the
"collation doesn't trace to a foreign Var, but it's default so we don't
really care" case.  I think the problem we've got is that the
non-foreign-Var code thinks that's what the definition is, but the rest
of the code isn't quite consistent with it.

In any case I think we want to end up with a clearer specification of
what the states mean.

                        regards, tom lane

Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:

Reply via email to