Re: Push down time-related SQLValue functions to foreign server - Mailing list pgsql-hackers

Hi.

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

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