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

From: "Dian M Fay" <dian(dot)m(dot)fay(at)gmail(dot)com>
To: "Dian M Fay" <dian(dot)m(dot)fay(at)gmail(dot)com>, "Tom Lane" <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: "David Steele" <david(at)pgmasters(dot)net>, "Georgios Kokolatos" <gkokolatos(at)protonmail(dot)com>, "PostgreSQL Developers" <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: [PATCH] postgres_fdw: suppress explicit casts in text:text comparisons (was: column option to override foreign types)
Date: 2021-08-05 02:38:24
Message-ID: CDB8B3UONXNW.CGCW6TBNME7B@lamia
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Sun Mar 7, 2021 at 2:37 AM EST, Dian M Fay wrote:
> > What'd be better, if we could do it, is to ship the clause in
> > the form
> > WHERE foreigncol = 'one'
> > that is, instead of plastering a cast on the Var, try to not put
> > any explicit cast on the constant. That fixes your original use
> > case better than what you've proposed, and I think it might be
> > possible to do it unconditionally instead of needing a hacky
> > column property to enable it. The reason this could be okay
> > is that it seems reasonable for postgres_fdw to rely on the
> > core parser's heuristic that an unknown-type literal is the
> > same type as what it's being compared to. So, if we are trying
> > to deparse something of the form "foreigncol operator constant",
> > and the foreigncol and constant are of the same type locally,
> > we could leave off the cast on the constant. (There might need
> > to be some restrictions about the operator taking those types
> > natively with no cast, not sure; also this doesn't apply to
> > constants that are going to be printed as non-string literals.)
> >
> > Slipping this heuristic into the code structure of deparse.c
> > might be rather messy, though. I've not looked at just how
> > to implement it.
>
> This doesn't look too bad from here, at least so far. The attached
> change adds a new const_showtype field to the deparse_expr_cxt, and
> passes that instead of the hardcoded 0 to deparseConst. deparseOpExpr
> modifies const_showtype if both sides of a binary operation are text,
> and resets it to 0 after the recursion.
>
> I restricted it to text-only after seeing a regression test fail: while
> deparsing `percentile_cont(c2/10::numeric)`, c2, an integer column, is a
> FuncExpr with a numeric return type. That matches the numeric 10, and
> without the explicit cast, integer-division-related havoc ensues. I
> don't know why it's a FuncExpr, and I don't know why it's not an int,
> but the constant is definitely a non-string, in any case.
>
> In the course of testing, I discovered that the @@ text-search operator
> works against textified enums on my stock 13.1 server (a "proper" enum
> column yields "operator does not exist"). I'm rather wary of actually
> trying to depend on that behavior, although it seems probably-safe in
> the same character set and collation.

hello again! My second version of this change (suppressing the cast
entirely as Tom suggested) seemed to slip under the radar back in March
and then other matters intervened. I'm still interested in making it
happen, though, and now that we're out of another commitfest it seems
like a good time to bring it back up. Here's a rebased patch to start.

Attachment Content-Type Size
0001-Suppress-explicit-casts-of-text-constants-in-postgre.patch text/plain 19.0 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message kuroda.hayato@fujitsu.com 2021-08-05 03:18:58 Allow escape in application_name (was: [postgres_fdw] add local pid to fallback_application_name)
Previous Message Amit Langote 2021-08-05 02:32:58 Re: Partition Check not updated when insert into a partition