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

From: Alexander Pyhalov <a(dot)pyhalov(at)postgrespro(dot)ru>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>, Zhihong Yu <zyu(at)yugabyte(dot)com>, Pgsql Hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Push down time-related SQLValue functions to foreign server
Date: 2022-01-18 08:52:25
Message-ID: b8612ae44251e6141bbcc4f485a1ee98@postgrespro.ru
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi.

Tom Lane писал 2022-01-18 02:08:
> 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.)

Yes, sure, is_foreign_param() is called only when is_foreign_expr() is
true.
Simplified this part.

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

I've moved logic to contain_mutable_functions_skip_sqlvalues(), it
uses the same subroutines as contain_mutable_functions().
Should we instead just add one more parameter to
contain_mutable_functions()?
I'm not sure that it's a good idea given that
contain_mutable_functions() seems to be an
external interface.

>
> 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've removed PREPARE tests, but GROUP BY test checks
foreign_grouping_ok()/is_foreign_param() path,
so I think it's useful.

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

OK. Let's drop it for now.

--
Best regards,
Alexander Pyhalov,
Postgres Professional

Attachment Content-Type Size
0001-SQLValue-functions-pushdown.patch text/x-diff 15.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Juan José Santamaría Flecha 2022-01-18 09:41:07 [PATCH] allow src/tools/msvc/clean.bat script to be called from the root of the source tree
Previous Message Amit Langote 2022-01-18 08:10:22 Re: generic plans and "initial" pruning