Re: [PATCH] postgres-fdw: column option to override foreign types

From: "Dian M Fay" <dian(dot)m(dot)fay(at)gmail(dot)com>
To: "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: column option to override foreign types
Date: 2021-03-07 07:37:53
Message-ID: C9QY452TQ2P9.2HYR4XTVUSTBP@lamia
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

> 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.

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Julien Rouhaud 2021-03-07 07:39:36 Re: pg_stat_statements oddity with track = all
Previous Message Julien Rouhaud 2021-03-07 07:28:10 Re: pg_stat_statements oddity with track = all