public inbox for [email protected]
help / color / mirror / Atom feedFrom: Tom Lane <[email protected]>
To: Alexander Pyhalov <[email protected]>
Cc: Ranier Vilela <[email protected]>
Cc: Zhihong Yu <[email protected]>
Cc: Pgsql Hackers <[email protected]>
Subject: Re: Push down time-related SQLValue functions to foreign server
Date: Mon, 17 Jan 2022 18:08:02 -0500
Message-ID: <[email protected]> (raw)
In-Reply-To: <[email protected]>
References: <[email protected]>
<CALNJ-vSp-jndrT8S40oUbNcbopv-PTROaQak2rMW=Be3MfmVbQ@mail.gmail.com>
<CAEudQArwB-C1FuShbcCAGdDqszt8Ho80eFHFLVE+Ki8Q=HnL_g@mail.gmail.com>
<[email protected]>
Alexander Pyhalov <[email protected]> writes:
>> Perhaps in a MACRO?
> Changed this check to a macro, also fixed condition in
> is_foreign_param() and added test for it.
> Also fixed comment in prepare_query_params().
I took a quick look at this. I'm unconvinced that you need the
TIME_RELATED_SQLVALUE_FUNCTION macro, mainly because I think testing
that in is_foreign_param() is pointless. The only way we'll be seeing
a SQLValueFunction in is_foreign_param() is if we decided it was
shippable, so you really don't need two duplicate tests.
(In the same vein, I would not bother with including a switch in
deparseSQLValueFunction that knows about these opcodes explicitly.
Just use the typmod field; exprTypmod() does.)
I also find it pretty bizarre that contain_unsafe_functions
isn't placed beside its one caller. Maybe that indicates that
is_foreign_expr is mis-located and should be in shippable.c.
More generally, it's annoying that you had to copy-and-paste
all of contain_mutable_functions to make this. That creates
a rather nasty maintenance hazard for future hackers, who will
need to keep these widely-separated functions in sync. Not
sure what to do about that though. Do we want to extend
contain_mutable_functions itself to cover this use-case?
The test cases seem a bit overkill --- what is the point of the
two nigh-identical PREPARE tests, or the GROUP BY test? If
anything is broken about GROUP BY, surely it's not specific
to this patch.
I'm not really convinced by the premise of 0002, particularly
this bit:
static bool
-contain_mutable_functions_checker(Oid func_id, void *context)
+contain_unsafe_functions_checker(Oid func_id, void *context)
{
- return (func_volatile(func_id) != PROVOLATILE_IMMUTABLE);
+ /* now() is stable, but we can ship it as it's replaced by parameter */
+ return !(func_volatile(func_id) == PROVOLATILE_IMMUTABLE || func_id == F_NOW);
}
The point of the check_functions_in_node callback API is to verify
the mutability of functions that are embedded in various sorts of
expression nodes ... but they might not be in a plain FuncExpr node,
which is the only case you'll deparse correctly. It might be that
now() cannot legally appear in any of the other node types that
check_functions_in_node knows about, but I'm not quite convinced
of that, and even less convinced that that'll stay true as we add
more expression node types. Also, if we commit this, for sure
some poor soul will try to expand the logic to some other stable
function that *can* appear in those contexts, and that'll be broken.
The implementation of converting now() to CURRENT_TIMESTAMP
seems like an underdocumented kluge, too.
On the whole I'm a bit inclined to drop 0002; I'm not sure it's
worth the trouble.
regards, tom lane
view thread (10+ messages) latest in thread
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: [email protected]
Cc: [email protected], [email protected], [email protected], [email protected]
Subject: Re: Push down time-related SQLValue functions to foreign server
In-Reply-To: <[email protected]>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox