Re: Getting rid of SQLValueFunction - Mailing list pgsql-hackers
| From | Corey Huinker |
|---|---|
| Subject | Re: Getting rid of SQLValueFunction |
| Date | |
| Msg-id | CADkLM=eywNqDLtVNx1zS+x4QMV6CmTmun0DG1n8os5rzv0ZkJA@mail.gmail.com Whole thread Raw |
| In response to | Getting rid of SQLValueFunction (Michael Paquier <michael@paquier.xyz>) |
| Responses |
Re: Getting rid of SQLValueFunction
(Michael Paquier <michael@paquier.xyz>)
|
| List | pgsql-hackers |
On Fri, Sep 30, 2022 at 2:04 AM Michael Paquier <michael@paquier.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.
pgsql-hackers by date: