Re: TEXT vs VARCHAR join qual push down diffrence, bug or expected? - Mailing list pgsql-hackers

<div dir="ltr">Hi Tom,<br /><div class="gmail_extra"><br /><div class="gmail_quote">On Tue, Sep 22, 2015 at 12:31 AM,
TomLane <span dir="ltr"><<a href="mailto:tgl@sss.pgh.pa.us" target="_blank">tgl@sss.pgh.pa.us</a>></span>
wrote:<br/><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid
rgb(204,204,204);padding-left:1ex"><spanclass=""><br /></span>I think you're blaming the wrong code; RelabelType is
handledbasically<br /> the same as most other cases.<br /><br /> It strikes me that this function is really going about
thingsthe wrong<br /> way.  Rather than trying to determine the output collation per se, what<br /> we ought to be
askingis "does every operator in the proposed expression<br /> have an input collation that can be traced to some
foreignVar further<br /> down in the expression"?  That is, given the example in hand,<br /><br />        
RelabelType(ForeignVar)= RelabelType(LocalVar)<br /><br /> the logic ought to be like "the ForeignVar has collation X,
andthat<br /> bubbles up without change through the RelabelType, and then the equals<br /> operator's inputcollation
matchesthat, so accept it --- regardless of<br /> where the other operand's collation came from exactly".  The key
point<br/> is that we want to validate operator input collations, not output<br /> collations, as having something to
dowith what the remote side would do.<br /><br /> This would represent a fairly significant rewrite of
foreign_expr_walker's<br/> collation logic; although I think the end result would be no more<br /> complicated,
possiblysimpler, than it is now.<br /><br />                         regards, tom lane<br /></blockquote></div><br
/><br/>IIUC, you are saying that collation check for output collation is not<br />necessary for all
OpExpr/FuncExpr/ArrayRefetc.<br />Should we remove code blocks like<br /><span
style="font-family:monospace,monospace">               collation = r->resultcollid;<br />                if
(collation== InvalidOid)<br />                    state = FDW_COLLATE_NONE;<br />                else if
(inner_cxt.state== FDW_COLLATE_SAFE &&<br />                         collation == inner_cxt.collation)<br />   
               state = FDW_COLLATE_SAFE;<br />                else<br />                    state =
FDW_COLLATE_UNSAFE;<br/></span><br />and just bubble up the collation and state to the next level?<br /><br />Here I
seethat, in the result collation validation, we missed the case when<br />result collation is default collation. For
foreignvar, we return collation<br />as is in inner context with the state set to SAFE. But in case of local var,<br
/>weare only allowing InvalidOid or Default oid for collation, however while<br />returning back, we set collation to
InvalidOidand state to NONE even for<br />default collation. I think we are missing something here.<br /><br />To
handlethis case, we need to either,<br />1. allow local var to set inner_cxt collation to what var actually has
(which<br/>will be either Invalid or DEFAULT) and set state to NONE if non collable or<br />set state to SAFE if
defaultcollation. Like:<br />  In T_Var, local var case<br /><span style="font-family:monospace,monospace">       
collation= var->varcollid;<br />        state = OidIsValid(collation) ? FDW_COLLATE_SAFE : FDW_COLLATE_NONE;<br
/></span><br/>OR<br />2. In above code block, which checks result collation, we need to handle<br />default collation.
Like:<br/><span style="font-family:monospace,monospace">                else if (collation == DEFAULT_COLLATION_OID)<br
/>                   state = inner_cxt.state;<br /></span><br />Let me know if I missed any.<br /><br />Thanks<br /><br
/>--<br /><div class="gmail_signature"><div dir="ltr">Jeevan B Chalke<br />Principal Software Engineer, Product
Development<br/>EnterpriseDB Corporation<br />The Enterprise PostgreSQL Company<br /><br /></div></div></div></div>