Re: Push down time-related SQLValue functions to foreign server

Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru> 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