Re: Getting rid of SQLValueFunction
On Fri, Dec 30, 2022 at 10:57:52AM +0900, Ian Lawrence Barwick wrote: > I noticed this commit (f193883f) introduces following regressions: > > postgres=# SELECT current_timestamp(7); > WARNING: TIMESTAMP(7) WITH TIME ZONE precision reduced to maximum > allowed, 6 > ERROR: timestamp(7) precision must be between 0 and 6 > > postgres=# SELECT localtimestamp(7); > WARNING: TIMESTAMP(7) precision reduced to maximum allowed, 6 > ERROR: timestamp(7) precision must be between 0 and 6 > > Suggested fix attached. The two changes in timestamp.c are fine, Now I can see that the same mistake was introduced in date.c. The WARNINGs were issued and the compilation went through the same way as the default, but they passed down an incorrect precision, so I have fixed all that. Coverage has been added for all four, while the patch proposed covered only two. -- Michael signature.asc Description: PGP signature
Re: Getting rid of SQLValueFunction
On Fri, Dec 30, 2022 at 10:57:52AM +0900, Ian Lawrence Barwick wrote: > I noticed this commit (f193883f) introduces following regressions: > > postgres=# SELECT current_timestamp(7); > WARNING: TIMESTAMP(7) WITH TIME ZONE precision reduced to maximum > allowed, 6 > ERROR: timestamp(7) precision must be between 0 and 6 > > postgres=# SELECT localtimestamp(7); > WARNING: TIMESTAMP(7) precision reduced to maximum allowed, 6 > ERROR: timestamp(7) precision must be between 0 and 6 > > Suggested fix attached. Thanks for the report, Ian. Will fix. -- Michael signature.asc Description: PGP signature
Re: Getting rid of SQLValueFunction
Hi
2022年11月21日(月) 18:39 Michael Paquier :
>
> On Sun, Nov 20, 2022 at 03:15:34PM -0800, Ted Yu wrote:
> > + * timestamp. These require a specific handling with their typmod is given
> > + * by the function caller through their SQL keyword.
> >
> > typo: typmod is given -> typmod given
> >
> > Other than the above, code looks good to me.
>
> Thanks for double-checking. I intended a different wording, actually,
> so fixed this one. And applied after an extra round of reviews.
I noticed this commit (f193883f) introduces following regressions:
postgres=# SELECT current_timestamp(7);
WARNING: TIMESTAMP(7) WITH TIME ZONE precision reduced to maximum
allowed, 6
ERROR: timestamp(7) precision must be between 0 and 6
postgres=# SELECT localtimestamp(7);
WARNING: TIMESTAMP(7) precision reduced to maximum allowed, 6
ERROR: timestamp(7) precision must be between 0 and 6
Suggested fix attached.
Regards
Ian Barwick
From b599765b222aacbc860ba43c42ec658ed1a8b989 Mon Sep 17 00:00:00 2001
From: Ian Barwick
Date: Fri, 30 Dec 2022 10:48:31 +0900
Subject: [PATCH v1] Fix [current_|local]timestamp precision reduction
Fixes regression introduced in f193883f.
---
src/backend/utils/adt/timestamp.c | 10 ++
src/test/regress/expected/expressions.out | 15 +++
src/test/regress/sql/expressions.sql | 3 +++
3 files changed, 20 insertions(+), 8 deletions(-)
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index 3f2508c0c4..b23cce1136 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -1606,10 +1606,7 @@ current_timestamp(PG_FUNCTION_ARGS)
int32 typmod = -1;
if (!PG_ARGISNULL(0))
- {
- typmod = PG_GETARG_INT32(0);
- anytimestamp_typmod_check(true, typmod);
- }
+ typmod = anytimestamp_typmod_check(true, PG_GETARG_INT32(0));
ts = GetCurrentTransactionStartTimestamp();
if (typmod >= 0)
@@ -1627,10 +1624,7 @@ sql_localtimestamp(PG_FUNCTION_ARGS)
int32 typmod = -1;
if (!PG_ARGISNULL(0))
- {
- typmod = PG_GETARG_INT32(0);
- anytimestamp_typmod_check(false, typmod);
- }
+ typmod = anytimestamp_typmod_check(false, PG_GETARG_INT32(0));
ts = timestamptz2timestamp(GetCurrentTransactionStartTimestamp());
if (typmod >= 0)
diff --git a/src/test/regress/expected/expressions.out b/src/test/regress/expected/expressions.out
index 28a20900f1..fd38830a77 100644
--- a/src/test/regress/expected/expressions.out
+++ b/src/test/regress/expected/expressions.out
@@ -50,6 +50,14 @@ SELECT length(current_timestamp::text) >= length(current_timestamp(0)::text);
t
(1 row)
+-- precision overflow
+SELECT current_timestamp = current_timestamp(7);
+WARNING: TIMESTAMP(7) WITH TIME ZONE precision reduced to maximum allowed, 6
+ ?column?
+--
+ t
+(1 row)
+
-- localtimestamp
SELECT now()::timestamp::text = localtimestamp::text;
?column?
@@ -57,6 +65,13 @@ SELECT now()::timestamp::text = localtimestamp::text;
t
(1 row)
+SELECT localtimestamp = localtimestamp(7);
+WARNING: TIMESTAMP(7) precision reduced to maximum allowed, 6
+ ?column?
+--
+ t
+(1 row)
+
-- current_role/user/user is tested in rolnames.sql
-- current database / catalog
SELECT current_catalog = current_database();
diff --git a/src/test/regress/sql/expressions.sql b/src/test/regress/sql/expressions.sql
index f9a0299d17..57dfb37745 100644
--- a/src/test/regress/sql/expressions.sql
+++ b/src/test/regress/sql/expressions.sql
@@ -21,8 +21,11 @@ SELECT now()::time(3)::text = localtime(3)::text;
SELECT current_timestamp = NOW();
-- precision
SELECT length(current_timestamp::text) >= length(current_timestamp(0)::text);
+-- precision overflow
+SELECT current_timestamp = current_timestamp(7);
-- localtimestamp
SELECT now()::timestamp::text = localtimestamp::text;
+SELECT localtimestamp = localtimestamp(7);
-- current_role/user/user is tested in rolnames.sql
base-commit: 388e80132c007a7528abc987e347e41432dc1542
--
2.31.1
Re: Getting rid of SQLValueFunction
On Sun, Nov 20, 2022 at 03:15:34PM -0800, Ted Yu wrote: > + * timestamp. These require a specific handling with their typmod is given > + * by the function caller through their SQL keyword. > > typo: typmod is given -> typmod given > > Other than the above, code looks good to me. Thanks for double-checking. I intended a different wording, actually, so fixed this one. And applied after an extra round of reviews. -- Michael signature.asc Description: PGP signature
Re: Getting rid of SQLValueFunction
On Sun, Nov 20, 2022 at 3:12 PM Michael Paquier wrote: > On Sun, Nov 20, 2022 at 08:21:10AM -0800, Ted Yu wrote: > > For get_func_sql_syntax(), the code for cases > > of F_CURRENT_TIME, F_CURRENT_TIMESTAMP, F_LOCALTIME and F_LOCALTIMESTAMP > is > > mostly the same. > > Maybe we can introduce a helper so that code duplication is reduced. > > It would. Thanks for the suggestion. > > Do you like something like the patch 0002 attached? This reduces a > bit the overall size of the patch. Both ought to be merged in the > same commit, still it is easier to see the simplification created. > -- > Michael > Hi, Thanks for the quick response. + * timestamp. These require a specific handling with their typmod is given + * by the function caller through their SQL keyword. typo: typmod is given -> typmod given Other than the above, code looks good to me. Cheers
Re: Getting rid of SQLValueFunction
On Sun, Nov 20, 2022 at 08:21:10AM -0800, Ted Yu wrote:
> For get_func_sql_syntax(), the code for cases
> of F_CURRENT_TIME, F_CURRENT_TIMESTAMP, F_LOCALTIME and F_LOCALTIMESTAMP is
> mostly the same.
> Maybe we can introduce a helper so that code duplication is reduced.
It would. Thanks for the suggestion.
Do you like something like the patch 0002 attached? This reduces a
bit the overall size of the patch. Both ought to be merged in the
same commit, still it is easier to see the simplification created.
--
Michael
From 3611cfa6f0171dcd65eeb88461f4c48989cd3f1a Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Sun, 20 Nov 2022 11:53:28 +0900
Subject: [PATCH v5 1/2] Replace SQLValueFunction by direct function calls
This impacts 9 patterns where the SQL grammar takes priority:
- localtime, that can take a typmod.
- localtimestamp, that can take a typmod.
- current_time, that can take a typmod.
- current_timestamp, that can take a typmod.
- current_date.
Five new entries are added to pg_proc to compensate the removal of
SQLValueFunction to keep compatibility (when a keyword is specified in a
SELECT or in a FROM clause without an alias), with tests to cover all
that.
XXX: bump catalog version.
Reviewed-by: Corey Huinker
Discussion: https://postgr.es/m/yzag3morycguu...@paquier.xyz
---
src/include/catalog/pg_proc.dat | 15 +++
src/include/executor/execExpr.h | 8 --
src/include/nodes/primnodes.h | 33 ---
src/include/utils/date.h | 4 -
src/include/utils/timestamp.h | 4 -
src/backend/catalog/system_functions.sql | 26 ++
src/backend/executor/execExpr.c | 11 ---
src/backend/executor/execExprInterp.c | 46 -
src/backend/jit/llvm/llvmjit_expr.c | 6 --
src/backend/jit/llvm/llvmjit_types.c | 1 -
src/backend/nodes/nodeFuncs.c | 27 +-
src/backend/optimizer/path/costsize.c | 1 -
src/backend/optimizer/util/clauses.c | 39 ++--
src/backend/parser/gram.y | 59 +++-
src/backend/parser/parse_expr.c | 52 ---
src/backend/parser/parse_target.c | 25 -
src/backend/utils/adt/date.c | 81 +---
src/backend/utils/adt/ruleutils.c | 109 +-
src/backend/utils/adt/timestamp.c | 72 --
src/backend/utils/misc/queryjumble.c | 9 --
src/test/regress/expected/expressions.out | 2 +-
src/test/regress/sql/expressions.sql | 2 +-
src/tools/pgindent/typedefs.list | 2 -
23 files changed, 245 insertions(+), 389 deletions(-)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index fd2559442e..35dc369728 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -1520,6 +1520,21 @@
{ oid => '9977', descr => 'system user name',
proname => 'system_user', provolatile => 's', prorettype => 'text',
proargtypes => '', prosrc => 'system_user' },
+{ oid => '9978', descr => 'current date',
+ proname => 'current_date', provolatile => 's', prorettype => 'date',
+ proargtypes => '', prosrc => 'current_date' },
+{ oid => '9979', descr => 'current time',
+ proname => 'current_time', provolatile => 's', prorettype => 'timetz',
+ proargtypes => 'int4', prosrc => 'current_time', proisstrict => 'f' },
+{ oid => '9980', descr => 'current timestamp',
+ proname => 'current_timestamp', provolatile => 's', prorettype => 'timestamptz',
+ proargtypes => 'int4', prosrc => 'current_timestamp', proisstrict => 'f' },
+{ oid => '9981', descr => 'local time',
+ proname => 'localtime', provolatile => 's', prorettype => 'time',
+ proargtypes => 'int4', prosrc => 'sql_localtime', proisstrict => 'f' },
+{ oid => '9982', descr => 'local timestamp',
+ proname => 'localtimestamp', provolatile => 's', prorettype => 'timestamp',
+ proargtypes => 'int4', prosrc => 'sql_localtimestamp', proisstrict => 'f' },
{ oid => '744',
proname => 'array_eq', prorettype => 'bool',
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index e14f15d435..0557302b92 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -170,7 +170,6 @@ typedef enum ExprEvalOp
EEOP_DISTINCT,
EEOP_NOT_DISTINCT,
EEOP_NULLIF,
- EEOP_SQLVALUEFUNCTION,
EEOP_CURRENTOFEXPR,
EEOP_NEXTVALUEEXPR,
EEOP_ARRAYEXPR,
@@ -416,12 +415,6 @@ typedef struct ExprEvalStep
FunctionCallInfo fcinfo_data_in;
} iocoerce;
- /* for EEOP_SQLVALUEFUNCTION */
- struct
- {
- SQLValueFunction *svf;
- } sqlvaluefunction;
-
/* for EEOP_NEXTVALUEEXPR */
struct
{
@@ -741,7 +734,6 @@ extern void ExecEvalParamExec(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
extern void ExecEvalParamExtern(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
-extern void ExecEvalSQLValueFunction(ExprState *state, ExprEvalStep *op);
extern
Re: Getting rid of SQLValueFunction
On Sat, Nov 19, 2022 at 7:01 PM Michael Paquier wrote: > On Fri, Nov 18, 2022 at 10:23:58AM +0900, Michael Paquier wrote: > > Please note that in order to avoid tweaks when choosing the attribute > > name of function call, this needs a total of 8 new catalog functions > > mapping to the SQL keywords, which is what the test added by 2e0d80c > > is about: > > - current_role > > - user > > - current_catalog > > - current_date > > - current_time > > - current_timestamp > > - localtime > > - localtimestamp > > > > Any objections? > > Hearing nothing, I have gone through 0001 again and applied it as > fb32748 to remove the dependency between names and SQLValueFunction. > Attached is 0002, to bring back the CI to a green state. > -- > Michael > Hi, For get_func_sql_syntax(), the code for cases of F_CURRENT_TIME, F_CURRENT_TIMESTAMP, F_LOCALTIME and F_LOCALTIMESTAMP is mostly the same. Maybe we can introduce a helper so that code duplication is reduced. Cheers
Re: Getting rid of SQLValueFunction
On Fri, Nov 18, 2022 at 10:23:58AM +0900, Michael Paquier wrote:
> Please note that in order to avoid tweaks when choosing the attribute
> name of function call, this needs a total of 8 new catalog functions
> mapping to the SQL keywords, which is what the test added by 2e0d80c
> is about:
> - current_role
> - user
> - current_catalog
> - current_date
> - current_time
> - current_timestamp
> - localtime
> - localtimestamp
>
> Any objections?
Hearing nothing, I have gone through 0001 again and applied it as
fb32748 to remove the dependency between names and SQLValueFunction.
Attached is 0002, to bring back the CI to a green state.
--
Michael
From 3611cfa6f0171dcd65eeb88461f4c48989cd3f1a Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Sun, 20 Nov 2022 11:53:28 +0900
Subject: [PATCH v4] Replace SQLValueFunction by direct function calls
This impacts 9 patterns where the SQL grammar takes priority:
- localtime, that can take a typmod.
- localtimestamp, that can take a typmod.
- current_time, that can take a typmod.
- current_timestamp, that can take a typmod.
- current_date.
Five new entries are added to pg_proc to compensate the removal of
SQLValueFunction to keep compatibility (when a keyword is specified in a
SELECT or in a FROM clause without an alias), with tests to cover all
that.
XXX: bump catalog version.
Reviewed-by: Corey Huinker
Discussion: https://postgr.es/m/yzag3morycguu...@paquier.xyz
---
src/include/catalog/pg_proc.dat | 15 +++
src/include/executor/execExpr.h | 8 --
src/include/nodes/primnodes.h | 33 ---
src/include/utils/date.h | 4 -
src/include/utils/timestamp.h | 4 -
src/backend/catalog/system_functions.sql | 26 ++
src/backend/executor/execExpr.c | 11 ---
src/backend/executor/execExprInterp.c | 46 -
src/backend/jit/llvm/llvmjit_expr.c | 6 --
src/backend/jit/llvm/llvmjit_types.c | 1 -
src/backend/nodes/nodeFuncs.c | 27 +-
src/backend/optimizer/path/costsize.c | 1 -
src/backend/optimizer/util/clauses.c | 39 ++--
src/backend/parser/gram.y | 59 +++-
src/backend/parser/parse_expr.c | 52 ---
src/backend/parser/parse_target.c | 25 -
src/backend/utils/adt/date.c | 81 +---
src/backend/utils/adt/ruleutils.c | 109 +-
src/backend/utils/adt/timestamp.c | 72 --
src/backend/utils/misc/queryjumble.c | 9 --
src/test/regress/expected/expressions.out | 2 +-
src/test/regress/sql/expressions.sql | 2 +-
src/tools/pgindent/typedefs.list | 2 -
23 files changed, 245 insertions(+), 389 deletions(-)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index fd2559442e..35dc369728 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -1520,6 +1520,21 @@
{ oid => '9977', descr => 'system user name',
proname => 'system_user', provolatile => 's', prorettype => 'text',
proargtypes => '', prosrc => 'system_user' },
+{ oid => '9978', descr => 'current date',
+ proname => 'current_date', provolatile => 's', prorettype => 'date',
+ proargtypes => '', prosrc => 'current_date' },
+{ oid => '9979', descr => 'current time',
+ proname => 'current_time', provolatile => 's', prorettype => 'timetz',
+ proargtypes => 'int4', prosrc => 'current_time', proisstrict => 'f' },
+{ oid => '9980', descr => 'current timestamp',
+ proname => 'current_timestamp', provolatile => 's', prorettype => 'timestamptz',
+ proargtypes => 'int4', prosrc => 'current_timestamp', proisstrict => 'f' },
+{ oid => '9981', descr => 'local time',
+ proname => 'localtime', provolatile => 's', prorettype => 'time',
+ proargtypes => 'int4', prosrc => 'sql_localtime', proisstrict => 'f' },
+{ oid => '9982', descr => 'local timestamp',
+ proname => 'localtimestamp', provolatile => 's', prorettype => 'timestamp',
+ proargtypes => 'int4', prosrc => 'sql_localtimestamp', proisstrict => 'f' },
{ oid => '744',
proname => 'array_eq', prorettype => 'bool',
diff --git a/src/include/executor/execExpr.h b/src/include/executor/execExpr.h
index e14f15d435..0557302b92 100644
--- a/src/include/executor/execExpr.h
+++ b/src/include/executor/execExpr.h
@@ -170,7 +170,6 @@ typedef enum ExprEvalOp
EEOP_DISTINCT,
EEOP_NOT_DISTINCT,
EEOP_NULLIF,
- EEOP_SQLVALUEFUNCTION,
EEOP_CURRENTOFEXPR,
EEOP_NEXTVALUEEXPR,
EEOP_ARRAYEXPR,
@@ -416,12 +415,6 @@ typedef struct ExprEvalStep
FunctionCallInfo fcinfo_data_in;
} iocoerce;
- /* for EEOP_SQLVALUEFUNCTION */
- struct
- {
- SQLValueFunction *svf;
- } sqlvaluefunction;
-
/* for EEOP_NEXTVALUEEXPR */
struct
{
@@ -741,7 +734,6 @@ extern void ExecEvalParamExec(ExprState *state, ExprEvalStep *op,
ExprContext *econtext);
extern void ExecEvalParamExtern(ExprState *state, ExprEvalStep
Re: Getting rid of SQLValueFunction
On Tue, Oct 25, 2022 at 02:20:12PM +0900, Michael Paquier wrote: > Attached is a rebased patch set, as of the conflicts from 2e0d80c. So, this patch set has been sitting in the CF app for a few weeks now, and I would like to apply them to remove a bit of code from the executor. Please note that in order to avoid tweaks when choosing the attribute name of function call, this needs a total of 8 new catalog functions mapping to the SQL keywords, which is what the test added by 2e0d80c is about: - current_role - user - current_catalog - current_date - current_time - current_timestamp - localtime - localtimestamp Any objections? -- Michael signature.asc Description: PGP signature
Re: Getting rid of SQLValueFunction
On Fri, Oct 21, 2022 at 02:27:07PM +0900, Michael Paquier wrote:
> I have looked at that, and the attribute mapping remains compatible
> with past versions once the appropriate pg_proc entries are added.
> The updated patch set attached does that (with a user() function as
> well to keep the code a maximum simple), with more tests to cover the
> attribute case mentioned upthread.
Attached is a rebased patch set, as of the conflicts from 2e0d80c.
--
Michael
From 1d2d6e4803f3ab3e9d2c29efcc8dee0f8a53d699 Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Tue, 25 Oct 2022 14:15:14 +0900
Subject: [PATCH v3 1/2] Remove from SQLValueFunction all the entries using
"name" as return type
This commit changes six SQL keywords to use COERCE_SQL_SYNTAX rather
than SQLValueFunction:
- CURRENT_ROLE
- CURRENT_USER
- USER
- SESSION_USER
- CURRENT_CATALOG
- CURRENT_SCHEMA
Among the six, "user", "current_role" and "current_catalog" require
specific SQL functions to allow ruleutils.c to map them to the SQL
keywords these require when using COERCE_SQL_SYNTAX. Having
pg_proc.proname match with the keyword ensures that the compatibility
remains the same when projecting any of these keywords in a FROM clause
to an attribute name when an alias is not specified. Tests are added to
make sure that the mapping happens from the SQL keyword is correct,
using a view defition that feeds on these keywords in one of the tests
introduced by 40c24bf (both in a SELECT clause and when using each
keyword in a FROM clause).
SQLValueFunction is reduced to half its contents after this change,
simplifying its logic a bit as there is no need to enforce a C collation
anymore. I have made a few performance tests, with a million-ish calls
to these keywords without seeing a difference in run-time or in perf
profiles. The remaining SQLValueFunctions relate to timestamps.
Bump catalog version.
Reviewed-by: Corey Huinker
Discussion: https://postgr.es/m/yzag3morycguu...@paquier.xyz
---
src/include/catalog/pg_proc.dat | 9 +++
src/include/nodes/primnodes.h | 8 +-
src/backend/executor/execExprInterp.c | 27
src/backend/nodes/nodeFuncs.c | 11 +++-
src/backend/parser/gram.y | 30 +-
src/backend/parser/parse_expr.c | 8 --
src/backend/parser/parse_target.c | 18 --
src/backend/utils/adt/ruleutils.c | 36 +--
8 files changed, 55 insertions(+), 92 deletions(-)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 62a5b8e655..241366fc8e 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -1505,6 +1505,15 @@
{ oid => '745', descr => 'current user name',
proname => 'current_user', provolatile => 's', prorettype => 'name',
proargtypes => '', prosrc => 'current_user' },
+{ oid => '9695', descr => 'current role name',
+ proname => 'current_role', provolatile => 's', prorettype => 'name',
+ proargtypes => '', prosrc => 'current_user' },
+{ oid => '9696', descr => 'user name',
+ proname => 'user', provolatile => 's', prorettype => 'name',
+ proargtypes => '', prosrc => 'current_user' },
+{ oid => '9697', descr => 'name of the current database',
+ proname => 'current_catalog', provolatile => 's', prorettype => 'name',
+ proargtypes => '', prosrc => 'current_database' },
{ oid => '746', descr => 'session user name',
proname => 'session_user', provolatile => 's', prorettype => 'name',
proargtypes => '', prosrc => 'session_user' },
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index f71f551782..f6dd27edcc 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1313,13 +1313,7 @@ typedef enum SQLValueFunctionOp
SVFOP_LOCALTIME,
SVFOP_LOCALTIME_N,
SVFOP_LOCALTIMESTAMP,
- SVFOP_LOCALTIMESTAMP_N,
- SVFOP_CURRENT_ROLE,
- SVFOP_CURRENT_USER,
- SVFOP_USER,
- SVFOP_SESSION_USER,
- SVFOP_CURRENT_CATALOG,
- SVFOP_CURRENT_SCHEMA
+ SVFOP_LOCALTIMESTAMP_N
} SQLValueFunctionOp;
typedef struct SQLValueFunction
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 9b9bbf00a9..6ebf5c287e 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -2495,15 +2495,10 @@ ExecEvalParamExtern(ExprState *state, ExprEvalStep *op, ExprContext *econtext)
void
ExecEvalSQLValueFunction(ExprState *state, ExprEvalStep *op)
{
- LOCAL_FCINFO(fcinfo, 0);
SQLValueFunction *svf = op->d.sqlvaluefunction.svf;
*op->resnull = false;
- /*
- * Note: current_schema() can return NULL. current_user() etc currently
- * cannot, but might as well code those cases the same way for safety.
- */
switch (svf->op)
{
case SVFOP_CURRENT_DATE:
@@ -2525,28 +2520,6 @@ ExecEvalSQLValueFunction(ExprState *state, ExprEvalStep *op)
case SVFOP_LOCALTIMESTAMP_N:
*op->resvalue =
Re: Getting rid of SQLValueFunction
On Fri, Oct 21, 2022 at 12:34:23PM +0900, Michael Paquier wrote:
> A sticky point is that this would need the creation of a pg_proc entry
> for "user" which is a generic word, or a shortcut around
> FigureColnameInternal(). The code gain overall still looks appealing
> in the executor, even if we do all that and the resulting backend code
> gets kind of nicer and easier to maintain long-term IMO.
I have looked at that, and the attribute mapping remains compatible
with past versions once the appropriate pg_proc entries are added.
The updated patch set attached does that (with a user() function as
well to keep the code a maximum simple), with more tests to cover the
attribute case mentioned upthread.
--
Michael
From 8d1a424e44f46f3107a6f0f066da22c19e4e4f3b Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Fri, 21 Oct 2022 14:06:03 +0900
Subject: [PATCH v2 1/2] Remove from SQLValueFunction all the entries using
"name" as return type
This commit changes six SQL keywords to use COERCE_SQL_SYNTAX rather
than SQLValueFunction:
- CURRENT_ROLE
- CURRENT_USER
- USER
- SESSION_USER
- CURRENT_CATALOG
- CURRENT_SCHEMA
Among the six, "user", "current_role" and "current_catalog" require
specific SQL functions to allow ruleutils.c to map them to the SQL
keywords these require when using COERCE_SQL_SYNTAX. Having
pg_proc.proname match with the keyword ensures that the compatibility
remains the same when projecting any of these keywords in a FROM clause
to an attribute name when an alias is not specified. Tests are added to
make sure that the mapping happens from the SQL keyword is correct,
using a view defition that feeds on these keywords in one of the tests
introduced by 40c24bf (both in a SELECT clause and when using each
keyword in a FROM clause).
SQLValueFunction is reduced to half its contents after this change,
simplifying its logic a bit as there is no need to enforce a C collation
anymore. I have made a few performance tests, with a million-ish calls
to these keywords without seeing a difference in run-time or in perf
profiles. The remaining SQLValueFunctions relate to timestamps.
Bump catalog version.
Reviewed-by: Corey Huinker
Discussion: https://postgr.es/m/yzag3morycguu...@paquier.xyz
---
src/include/catalog/catversion.h | 2 +-
src/include/catalog/pg_proc.dat | 9 ++
src/include/nodes/primnodes.h | 8 +
src/backend/executor/execExprInterp.c | 27 -
src/backend/nodes/nodeFuncs.c | 11 ++-
src/backend/parser/gram.y | 30 ++
src/backend/parser/parse_expr.c | 8 -
src/backend/parser/parse_target.c | 18 ---
src/backend/utils/adt/ruleutils.c | 36 +++---
src/test/regress/expected/create_view.out | 37 +--
src/test/regress/sql/create_view.sql | 15 -
11 files changed, 105 insertions(+), 96 deletions(-)
diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h
index 4c930c189b..032a429345 100644
--- a/src/include/catalog/catversion.h
+++ b/src/include/catalog/catversion.h
@@ -57,6 +57,6 @@
*/
/* mmddN */
-#define CATALOG_VERSION_NO 202210141
+#define CATALOG_VERSION_NO 202210211
#endif
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 62a5b8e655..241366fc8e 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -1505,6 +1505,15 @@
{ oid => '745', descr => 'current user name',
proname => 'current_user', provolatile => 's', prorettype => 'name',
proargtypes => '', prosrc => 'current_user' },
+{ oid => '9695', descr => 'current role name',
+ proname => 'current_role', provolatile => 's', prorettype => 'name',
+ proargtypes => '', prosrc => 'current_user' },
+{ oid => '9696', descr => 'user name',
+ proname => 'user', provolatile => 's', prorettype => 'name',
+ proargtypes => '', prosrc => 'current_user' },
+{ oid => '9697', descr => 'name of the current database',
+ proname => 'current_catalog', provolatile => 's', prorettype => 'name',
+ proargtypes => '', prosrc => 'current_database' },
{ oid => '746', descr => 'session user name',
proname => 'session_user', provolatile => 's', prorettype => 'name',
proargtypes => '', prosrc => 'session_user' },
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 40661334bb..e818231e15 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1313,13 +1313,7 @@ typedef enum SQLValueFunctionOp
SVFOP_LOCALTIME,
SVFOP_LOCALTIME_N,
SVFOP_LOCALTIMESTAMP,
- SVFOP_LOCALTIMESTAMP_N,
- SVFOP_CURRENT_ROLE,
- SVFOP_CURRENT_USER,
- SVFOP_USER,
- SVFOP_SESSION_USER,
- SVFOP_CURRENT_CATALOG,
- SVFOP_CURRENT_SCHEMA
+ SVFOP_LOCALTIMESTAMP_N
} SQLValueFunctionOp;
typedef struct SQLValueFunction
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index
Re: Getting rid of SQLValueFunction
On Thu, Oct 20, 2022 at 11:10:22PM -0400, Tom Lane wrote:
> The entire point of SQLValueFunction IMO was to hide the underlying
> implementation(s). Replacing it with something that leaks
> implementation details does not seem like a step forward.
Hmm.. Okay, thanks. So this just comes down that I am going to need
one different pg_proc entry per SQL keyword, then, or this won't fly
far. For example, note that on HEAD or with the patch, a view with a
SQL keyword in a FROM clause translates the same way with quotes
applied in the same places, as of:
=# create view test as select (SELECT * FROM CURRENT_USER) as cu;
CREATE VIEW
=# select pg_get_viewdef('test', true);
pg_get_viewdef
-
SELECT ( SELECT "current_user"."current_user" +
FROM CURRENT_USER "current_user"("current_user")) AS cu;
(1 row)
A sticky point is that this would need the creation of a pg_proc entry
for "user" which is a generic word, or a shortcut around
FigureColnameInternal(). The code gain overall still looks appealing
in the executor, even if we do all that and the resulting backend code
gets kind of nicer and easier to maintain long-term IMO.
--
Michael
signature.asc
Description: PGP signature
Re: Getting rid of SQLValueFunction
Michael Paquier writes: > But the patch enforces the attribute name to be the underlying > function name, switching the previous "current_catalog" to > "current_database". The entire point of SQLValueFunction IMO was to hide the underlying implementation(s). Replacing it with something that leaks implementation details does not seem like a step forward. regards, tom lane
Re: Getting rid of SQLValueFunction
On Wed, Oct 19, 2022 at 03:45:48PM +0900, Michael Paquier wrote: > With this in mind, would somebody complain if I commit that? That's a > nice reduction in code, while completing the work done in 40c24bf: > 25 files changed, 338 insertions(+), 477 deletions(-) On second look, there is something I have underestimated here with FigureColnameInternal(). This function would create an attribute name based on the SQL keyword given in input. For example, on HEAD we would get that: =# SELECT * FROM CURRENT_CATALOG; current_catalog - postgres (1 row) But the patch enforces the attribute name to be the underlying function name, switching the previous "current_catalog" to "current_database". For example: =# SELECT * FROM CURRENT_CATALOG; current_database -- postgres (1 row) I am not sure how much it matters in practice, but this could break some queries. One way to tackle that is to extend FigureColnameInternal() so as we use a compatible name when the node is a T_FuncCall, but that won't be entirely water-proof as long as there is not a one-one mapping between the SQL keywords and the underlying function names, aka we would need a current_catalog. "user" would be also too generic as a catalog function name, so we should name its proc entry to a pg_user anyway, requiring a shortcut in FigureColnameInternal(). Or perhaps I am worrying too much and keeping the code simpler is better? Does the SQL specification require that the attribute name has to match its SQL keyword when specified in a FROM clause when there is no aliases? Thoughts? -- Michael signature.asc Description: PGP signature
Re: Getting rid of SQLValueFunction
(Adding Tom in CC, in case.) On Tue, Oct 18, 2022 at 04:35:33PM -0400, Corey Huinker wrote: > 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. The message is the same between HEAD and the patch and these have been around for a long time, except that we would see it at parsing time on HEAD, and at executor time with the patch. I would not mind changing if there are better ideas than what's used now, of course ;) > 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. Once the timestamp functions are removed, SQLValueFunction is just dead code so including its removal in 0002 does not change much in my opinion. An other thing I had on my list for this patch was to check its performance impact. So I have spent some time having a look at the perf profiles produced on HEAD and with the patch using queries like SELECT current_role FROM generate_series(1,N) where N > 10M and I have not noticed any major differences in runtime or in the profiles, at the difference that we don't have anymore SQLValueFunction() and its internal functions called, hence they are missing from the stacks, but that's the whole point of the patch. With this in mind, would somebody complain if I commit that? That's a nice reduction in code, while completing the work done in 40c24bf: 25 files changed, 338 insertions(+), 477 deletions(-) Thanks, -- Michael signature.asc Description: PGP signature
Re: Getting rid of SQLValueFunction
On Fri, Sep 30, 2022 at 2:04 AM Michael Paquier 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.
Getting rid of SQLValueFunction
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
From 5eaabb5c306b06df2c3b0d18f075adb96074001f Mon Sep 17 00:00:00 2001
From: Michael Paquier
Date: Fri, 30 Sep 2022 10:48:22 +0900
Subject: [PATCH 1/2] Remove from SQLValueFunctionOp all name-based functions
This includes six functions, moved to be COERCE_SQL_SYNTAX:
- current_role
- role
- current_catalog
- current_schema
- current_database
- current_user
"user" and "current_role" need specific functions to allow ruleutils.c
to map to the expected SQL grammar these require. SQLValueFunctionOp is
reduced to half its contents.
---
src/include/catalog/pg_proc.dat | 6
src/include/nodes/primnodes.h | 8 +
src/backend/executor/execExprInterp.c | 27 -
src/backend/nodes/nodeFuncs.c | 4 +--
src/backend/parser/gram.y | 30 +++
src/backend/parser/parse_expr.c | 8 -
src/backend/parser/parse_target.c | 18
src/backend/utils/adt/ruleutils.c | 36 +++
src/test/regress/expected/create_view.out | 12
src/test/regress/sql/create_view.sql | 6
10 files changed, 68 insertions(+), 87 deletions(-)
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 68bb032d3e..414d752f9d 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -1505,6 +1505,12 @@
{ oid => '745', descr => 'current user name',
proname => 'current_user', provolatile => 's', prorettype => 'name',
proargtypes => '', prosrc => 'current_user' },
+{ oid => '9695', descr => 'current role name',
+ proname => 'current_role', provolatile => 's', prorettype => 'name',
+ proargtypes => '', prosrc => 'current_user' },
+{ oid => '9696', descr => 'user name',
+ proname => 'user', provolatile => 's', prorettype => 'name',
+ proargtypes => '', prosrc => 'current_user' },
{ oid => '746', descr => 'session user name',
proname => 'session_user', provolatile => 's', prorettype => 'name',
proargtypes => '', prosrc => 'session_user' },
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 40661334bb..e818231e15 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1313,13 +1313,7 @@ typedef enum SQLValueFunctionOp
SVFOP_LOCALTIME,
SVFOP_LOCALTIME_N,
SVFOP_LOCALTIMESTAMP,
- SVFOP_LOCALTIMESTAMP_N,
- SVFOP_CURRENT_ROLE,
- SVFOP_CURRENT_USER,
- SVFOP_USER,
- SVFOP_SESSION_USER,
- SVFOP_CURRENT_CATALOG,
- SVFOP_CURRENT_SCHEMA
+ SVFOP_LOCALTIMESTAMP_N
} SQLValueFunctionOp;
typedef struct SQLValueFunction
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 9b9bbf00a9..6ebf5c287e 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -2495,15 +2495,10 @@ ExecEvalParamExtern(ExprState *state,
