Hi Tom,
On Tue, Sep 22, 2015 at 12:31 AM, Tom Lane <t...@sss.pgh.pa.us> wrote:
>
> I think you're blaming the wrong code; RelabelType is handled basically
> the same as most other cases.
>
> It strikes me that this function is really going about things the wrong
> way. Rather than trying to determine the output collation per se, what
> we ought to be asking is "does every operator in the proposed expression
> have an input collation that can be traced to some foreign Var further
> down in the expression"? That is, given the example in hand,
>
> RelabelType(ForeignVar) = RelabelType(LocalVar)
>
> the logic ought to be like "the ForeignVar has collation X, and that
> bubbles up without change through the RelabelType, and then the equals
> operator's inputcollation matches that, so accept it --- regardless of
> where the other operand's collation came from exactly". The key point
> is that we want to validate operator input collations, not output
> collations, as having something to do with what the remote side would do.
>
> This would represent a fairly significant rewrite of foreign_expr_walker's
> collation logic; although I think the end result would be no more
> complicated, possibly simpler, than it is now.
>
> regards, tom lane
>
IIUC, you are saying that collation check for output collation is not
necessary for all OpExpr/FuncExpr/ArrayRef etc.
Should we remove code blocks 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
state = FDW_COLLATE_UNSAFE;
and just bubble up the collation and state to the next level?
Here I see that, in the result collation validation, we missed the case when
result collation is default collation. For foreign var, we return collation
as is in inner context with the state set to SAFE. But in case of local var,
we are only allowing InvalidOid or Default oid for collation, however while
returning back, we set collation to InvalidOid and state to NONE even for
default collation. I think we are missing something here.
To handle this case, we need to either,
1. allow local var to set inner_cxt collation to what var actually has
(which
will be either Invalid or DEFAULT) and set state to NONE if non collable or
set state to SAFE if default collation. Like:
In T_Var, local var case
collation = var->varcollid;
state = OidIsValid(collation) ? FDW_COLLATE_SAFE : FDW_COLLATE_NONE;
OR
2. In above code block, which checks result collation, we need to handle
default collation. Like:
else if (collation == DEFAULT_COLLATION_OID)
state = inner_cxt.state;
Let me know if I missed any.
Thanks
--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company
