Re: Getting rid of SQLValueFunction

On Fri, Sep 30, 2022 at 2:04 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:

> Hi all,
>
> I have bumped a few days ago on the fact that COERCE_SQL_SYNTAX
> (introduced by 40c24bf) and SQLValueFunction are around to do the
> exact same thing, as known as enforcing single-function calls with
> dedicated SQL keywords. For example, keywords like SESSION_USER,
> CURRENT_DATE, etc. go through SQLValueFunction and rely on the parser
> to set a state that gets then used in execExprInterp.c. And it is
> rather easy to implement incorrect SQLValueFunctions, as these rely on
> more hardcoded assumptions in the parser and executor than the
> equivalent FuncCalls (like collation to assign when using a text-like
> SQLValueFunctions).
>
> There are two categories of single-value functions:
> - The ones returning names, where we enforce a C collation in two
> places of the code (current_role, role, current_catalog,
> current_schema, current_database, current_user), even if
> get_typcollation() should do that for name types.
> - The ones working on time, date and timestamps (localtime[stamp],
> current_date, current_time[stamp]), for 9 patterns as these accept an
> optional typmod.
>
> I have dug into the possibility to unify all that with a single
> interface, and finish with the attached patch set which is a reduction
> of code, where all the SQLValueFunctions are replaced by a set of
> FuncCalls:
> 25 files changed, 338 insertions(+), 477 deletions(-)
>
> 0001 is the move done for the name-related functions, cleaning up two
> places in the executor when a C collation is assigned to those
> function expressions. 0002 is the remaining cleanup for the
> time-related ones, moving a set of parser-side checks to the execution
> path within each function, so as all this knowledge is now local to
> each file holding the date and timestamp types. Most of the gain is
> in 0002, obviously.
>
> The pg_proc entries introduced for the sake of the move use the same
> name as the SQL keywords. These should perhaps be prefixed with a
> "pg_" at least. There would be an exception with pg_localtime[stamp],
> though, where we could use a pg_localtime[stamp]_sql for the function
> name for prosrc. I am open to suggestions for these names.
>
> Thoughts?
> --
> Michael
>

I like this a lot. Deleted code is debugged code.

Patch applies and passes make check-world.

No trace of SQLValueFunction is left in the codebase, at least according to
`git grep -l`.

I have only one non-nitpick question about the code:

+ /*
+ * we're not too tense about good error message here because grammar
+ * shouldn't allow wrong number of modifiers for TIME
+ */
+ if (n != 1)
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("invalid type modifier")));

I agree that we shouldn't spend too much effort on a good error message
here, but perhaps we should have the message mention that it is
date/time-related? A person git-grepping for this error message will get 4
hits in .c files (date.c, timestamp.c, varbit.c, varchar.c) so even a
slight variation in the message could save them some time.

This is an extreme nitpick, but the patchset seems like it should have been
1 file or 3 (remove name functions, remove time functions, remove
SQLValueFunction infrastructure), but that will only matter in the unlikely
case that we find a need for SQLValueFunction but we want to leave the
timestamp function as COERCE_SQL_SYNTAX.