Re: [PATCH] postgres_fdw: suppress explicit casts in text:text comparisons (was: column option to override foreign types)

"Dian M Fay" <dian.m.fay@gmail.com> writes:
> Thanks Tom, that makes way more sense! I've attached a new patch which
> tests operands and makes sure one side is a Const before feeding it to
> deparseConst with a new showtype code, -2. The one regression is gone,
> but I've left a couple of test output discrepancies for now which
> showcase lost casts on the following predicates:

> * date(c5) = '1970-01-17'::date
> * ctid = '(0,2)'::tid

> These aren't exactly failures -- both implicit string comparisons work
> just fine -- but I don't know Postgres well enough to be sure that
> that's true more generally.

These seem fine to me.  The parser heuristic that we're relying on
acts at the level of the operator --- it doesn't really care whether
the other input argument is a simple Var or not.

Note that we're *not* doing an "implicit string comparison" in either
case.  The point here is that the remote parser will resolve the
unknown-type literal as being the same type as the other operator input,
that is date or tid in these two cases.

That being the goal, I think you don't have the logic right at all,
even if it happens to accidentally work in the tested cases.  We
can only drop the cast if it's a binary operator and the two inputs
are of the same type.  Testing "leftType == form->oprleft" is pretty
close to a no-op, because the input will have been coerced to the
operator's expected type.  And the code as you had it could do
indefensible things with a unary operator.  (It's probably hard to
get here with a unary operator applied to a constant, but I'm not
sure it's impossible.)

Attached is a rewrite that does what I think we want to do, and
also adds comments because there weren't any.

Now that I've looked this over I'm starting to feel uncomfortable
again, because we can't actually be quite sure about how the remote
parser's heuristic will act.  What we're checking is that leftType
and rightType match, but that condition is applied to the inputs
*after implicit type coercion to the operator's input types*.
We can't be entirely sure about what our parser saw to begin with.
Perhaps it'd be a good idea to strip any implicit coercions on
the non-Const input before checking its type.  I'm not sure how
much that helps though.  For one thing, by the time this code
sees the expression, eval_const_expressions could have collapsed
coercion steps in a way that obscures how it looked originally.
For another thing, in the cases we're interested in, it's kind of
a stretch to suppose that implicit coercions applied locally are
a good model of the way things will look to the remote parser.

So I'm feeling a bit itchy.  I'm still willing to push forward
with this, but I won't be terribly surprised if it breaks cases
that ought to work and we end up having to revert it.

			regards, tom lane