Underscore in positional parameters?

Started by Erik Wienholdover 1 year ago26 messages

ewie@ewie.name

over 1 year ago

1 attachment(s)

I noticed that we (kind of) accept underscores in positional parameters.
For example, this works:

=> PREPARE p1 AS SELECT $1_2;
PREPARE
=> EXECUTE p1 (123);
?column?
----------
123
(1 row)

Parameter $1_2 is taken as $1 because in rule {param} in scan.l we get
the parameter number with atol which stops at the underscore. That's a
regression in faff8f8e47f. Before that commit, $1_2 resulted in
"ERROR: trailing junk after parameter".

I can't tell which fix is the way to go: (1) accept underscores without
using atol, or (2) just forbid underscores. Any ideas?

atol can be replaced with pg_strtoint32_safe to handle the underscores.
This also avoids atol's undefined behavior on overflows. AFAICT,
positional parameters are not part of the SQL standard, so nothing
prevents us from accepting underscores here as well. The attached patch
does that and also adds a test case.

But reverting {param} to its old form to forbid underscores also makes
sense. That is:

param \${decdigit}+
param_junk \${decdigit}+{ident_start}

It seems very unlikely that anybody uses that many parameters and still
cares about readability to use underscores. But maybe users simply
expect that underscores are valid here as well.

--
Erik

Attachments:

0001-Fix-parsing-of-positional-parameters-with-underscore.patchtext/x-diff; charset=us-asciiDownload

From 0f319818360cdd26e96198ea7fcaba1b8298f264 Mon Sep 17 00:00:00 2001
From: Erik Wienhold <ewie@ewie.name>
Date: Tue, 14 May 2024 04:14:08 +0200
Subject: [PATCH] Fix parsing of positional parameters with underscores

Underscores were added to numeric literals in faff8f8e47.  That also
introduced a regression whereby positional parameters accepted
underscores as well.  But such parameter numbers were not parsed
correctly by atol which stops at the first non-digit character.  So
parameter $1_2 would be taken as just $1.  Fix that by replacing atol
with pg_strtoint32_safe.  Thereby we also avoid the undefined behavior
of atol on overflows.
---
 src/backend/parser/scan.l                | 5 ++++-
 src/test/regress/expected/numerology.out | 1 +
 src/test/regress/sql/numerology.sql      | 2 ++
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index 5eaadf53b2..ebb7ace9ca 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -992,7 +992,10 @@ other			.
 
 {param}			{
 					SET_YYLLOC();
-					yylval->ival = atol(yytext + 1);
+					ErrorSaveContext escontext = {T_ErrorSaveContext};
+					yylval->ival = pg_strtoint32_safe(yytext + 1, (Node *) &escontext);
+					if (escontext.error_occurred)
+						yyerror("parameter too large");
 					return PARAM;
 				}
 {param_junk}	{
diff --git a/src/test/regress/expected/numerology.out b/src/test/regress/expected/numerology.out
index f662a5050a..3c1308e22c 100644
--- a/src/test/regress/expected/numerology.out
+++ b/src/test/regress/expected/numerology.out
@@ -297,6 +297,7 @@ SELECT 1_000.5e0_1;
     10005
 (1 row)
 
+PREPARE p2 AS SELECT $0_1;
 -- error cases
 SELECT _100;
 ERROR:  column "_100" does not exist
diff --git a/src/test/regress/sql/numerology.sql b/src/test/regress/sql/numerology.sql
index 1941c58e68..11af76828d 100644
--- a/src/test/regress/sql/numerology.sql
+++ b/src/test/regress/sql/numerology.sql
@@ -77,6 +77,8 @@ SELECT 1_000.;
 SELECT .000_005;
 SELECT 1_000.5e0_1;
 
+PREPARE p2 AS SELECT $0_1;
+
 -- error cases
 SELECT _100;
 SELECT 100_;
-- 
2.45.0