|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|
|Views:||Raw Message | Whole Thread | Download mbox | Resend email|
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
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
I'm not sure that it's a good idea given that
contain_mutable_functions() seems to be an
> 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
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.
|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|