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 : FDW_COLLATE_NONE;
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
