Re: Underscore in positional parameters?

[Michael Paquier]

On Tue, May 14, 2024 at 05:18:24AM +0200, Erik Wienhold wrote:
> 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".

Indeed, the behavior of HEAD is confusing.  "1_2" means 12 as a
constant in a query, not 1, but HEAD implies 1 in the context of
PREPARE here.

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

Does the SQL specification tell anything about the way parameters
should be marked?  Not everything out there uses dollar-marked
parameters, so I guess that the answer to my question is no.  My take
is all these cases should be rejected for params, only apply to
numeric and integer constants in the queries.

> 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.

Adding Dean in CC as the committer of faff8f8e47f, Peter E for the SQL
specification part, and an open item.
--
Michael


signature.asc
Description: PGP signature

Re: Underscore in positional parameters?

[Dean Rasheed]

On Tue, 14 May 2024 at 07:43, Michael Paquier  wrote:
>
> On Tue, May 14, 2024 at 05:18:24AM +0200, Erik Wienhold wrote:
> > 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".
>
> Indeed, the behavior of HEAD is confusing.  "1_2" means 12 as a
> constant in a query, not 1, but HEAD implies 1 in the context of
> PREPARE here.
>
> > I can't tell which fix is the way to go: (1) accept underscores without
> > using atol, or (2) just forbid underscores.  Any ideas?
>
> Does the SQL specification tell anything about the way parameters
> should be marked?  Not everything out there uses dollar-marked
> parameters, so I guess that the answer to my question is no.  My take
> is all these cases should be rejected for params, only apply to
> numeric and integer constants in the queries.
>
> Adding Dean in CC as the committer of faff8f8e47f, Peter E for the SQL
> specification part, and an open item.

I'm sure that this wasn't intentional -- I think we just failed to
notice that "param" also uses "decinteger" in the scanner. Taking a
quick look, there don't appear to be any other uses of "decinteger",
so at least it only affects params.

Unless the spec explicitly says otherwise, I agree that we should
reject this, as we used to do, and add a comment saying that it's
intentionally not supported. I can't believe it would ever be useful,
and the current behaviour is clearly broken.

I've moved this to "Older bugs affecting stable branches", since it
came in with v16.

Regards,
Dean

Re: Underscore in positional parameters?

[Tom Lane]

Dean Rasheed  writes:
> On Tue, 14 May 2024 at 07:43, Michael Paquier  wrote:
>> On Tue, May 14, 2024 at 05:18:24AM +0200, Erik Wienhold wrote:
>>> 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'm sure that this wasn't intentional -- I think we just failed to
> notice that "param" also uses "decinteger" in the scanner. Taking a
> quick look, there don't appear to be any other uses of "decinteger",
> so at least it only affects params.

> Unless the spec explicitly says otherwise, I agree that we should
> reject this, as we used to do, and add a comment saying that it's
> intentionally not supported. I can't believe it would ever be useful,
> and the current behaviour is clearly broken.

+1, let's put this back the way it was.

regards, tom lane

Re: Underscore in positional parameters?

[Erik Wienhold]

On 2024-05-14 16:40 +0200, Tom Lane wrote:
> Dean Rasheed  writes:
> > On Tue, 14 May 2024 at 07:43, Michael Paquier  wrote:
> >> On Tue, May 14, 2024 at 05:18:24AM +0200, Erik Wienhold wrote:
> >>> 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'm sure that this wasn't intentional -- I think we just failed to
> > notice that "param" also uses "decinteger" in the scanner. Taking a
> > quick look, there don't appear to be any other uses of "decinteger",
> > so at least it only affects params.
> 
> > Unless the spec explicitly says otherwise, I agree that we should
> > reject this, as we used to do, and add a comment saying that it's
> > intentionally not supported. I can't believe it would ever be useful,
> > and the current behaviour is clearly broken.
> 
> +1, let's put this back the way it was.

I split the change in two independent patches:

Patch 0001 changes rules param and param_junk to only accept digits 0-9.

Patch 0002 replaces atol with pg_strtoint32_safe in the backend parser
and strtoint in ECPG.  This fixes overflows like:

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

=> PREPARE p2 AS SELECT $2147483648;
ERROR:  there is no parameter $-2147483648
LINE 1: PREPARE p2 AS SELECT $2147483648;

It now returns this error:

=> PREPARE p1 AS SELECT $4294967297;
ERROR:  parameter too large at or near $4294967297

=> PREPARE p2 AS SELECT $2147483648;
ERROR:  parameter too large at or near $2147483648

-- 
Erik
>From a3a3d845bbc60cb5ff1bc510205c84ab3bdeddbf Mon Sep 17 00:00:00 2001
From: Erik Wienhold 
Date: Tue, 14 May 2024 14:18:59 +0200
Subject: [PATCH v2 1/2] Forbid underscore in positional parameters

Underscores were added to numeric literals in faff8f8e47.  In an
oversight, this change also affected the positional parameters rule
which use the same production for its digits.  Underscores are not
necessary for positional parameters, so revert that rule to its old form
that only accepts digits 0-9.
---
 src/backend/parser/scan.l| 5 +++--
 src/fe_utils/psqlscan.l  | 5 +++--
 src/interfaces/ecpg/preproc/pgc.l| 5 +++--
 src/test/regress/expected/numerology.out | 4 
 src/test/regress/sql/numerology.sql  | 1 +
 5 files changed, 14 insertions(+), 6 deletions(-)

diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index 5eaadf53b2..b499975e9c 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -419,8 +419,9 @@ bininteger_junk	{bininteger}{ident_start}
 numeric_junk	{numeric}{ident_start}
 real_junk		{real}{ident_start}
 
-param			\${decinteger}
-param_junk		\${decinteger}{ident_start}
+/* Positional parameters don't accept underscores. */
+param			\${decdigit}+
+param_junk		\${decdigit}+{ident_start}
 
 other			.
 
diff --git a/src/fe_utils/psqlscan.l b/src/fe_utils/psqlscan.l
index c9df0594fd..c6d02439ab 100644
--- a/src/fe_utils/psqlscan.l
+++ b/src/fe_utils/psqlscan.l
@@ -355,8 +355,9 @@ bininteger_junk	{bininteger}{ident_start}
 numeric_junk	{numeric}{ident_start}
 real_junk		{real}{ident_start}
 
-param			\${decinteger}
-param_junk		\${decinteger}{ident_start}
+/* Positional parameters don't accept underscores. */
+param			\${decdigit}+
+param_junk		\${decdigit}+{ident_start}
 
 /* psql-specific: characters allowed in variable names */
 variable_char	[A-Za-z\200-\377_0-9]
diff --git a/src/interfaces/ecpg/preproc/pgc.l b/src/interfaces/ecpg/preproc/pgc.l
index bcfbd0978b..d117cafce6 100644
--- a/src/interfaces/ecpg/preproc/pgc.l
+++ b/src/interfaces/ecpg/preproc/pgc.l
@@ -388,8 +388,9 @@ bininteger_junk	{bininteger}{ident_start}
 numeric_junk	{numeric}{ident_start}
 real_junk		{real}{ident_start}
 
-param			\${decinteger}
-param_junk		\${decinteger}{ident_start}
+/* Positional parameters don't accept underscores. */
+param			\${decdigit}+
+param_junk		\${decdigit}+{ident_start}
 
 /* special characters for other dbms */
 /* we have to react differently in compat mode */
diff --git a/src/test/regress/expected/numerology.out b/src/test/regress/expected/numerology.out
index f662a5050a..c8196d2c85 100644
--- a/src/test/regress/expected/numerology.out
+++ b/src/test/regress/expected/numerology.out
@@ -330,6 +330,10 @@ SELECT 1_000.5e_1;
 ERROR:  trailing junk after numeric literal at or near "1_000.5e"
 LINE 1: SELECT 1_000.5e_1;
^
+PREPARE p1 AS SELECT $0_1;
+ERROR:  trailing junk after parameter at or near "$0_"
+LINE 1: PREPARE p1 AS SELECT $0_1;
+ ^
 --
 -- Test implicit type conversions
 -- This fails for Postgres v6.1 (and earlier?)
diff --git a/src/test/regress/sql/numerology.sql b/src/test/regress/sql/numerology.sql
index 1941

Re: Underscore in positional parameters?

[Michael Paquier]

On Tue, May 14, 2024 at 10:51:41AM +0100, Dean Rasheed wrote:
> I've moved this to "Older bugs affecting stable branches", since it
> came in with v16.

Oops, thanks for fixing.  I've somewhat missed that b2d47928908d was
in REL_16_STABLE.
--
Michael


signature.asc
Description: PGP signature

Re: Underscore in positional parameters?

[Michael Paquier]

On Tue, May 14, 2024 at 06:07:51PM +0200, Erik Wienhold wrote:
> I split the change in two independent patches:

The split makes sense to me.

> Patch 0001 changes rules param and param_junk to only accept digits 0-9.

-param  \${decinteger}
-param_junk \${decinteger}{ident_start}
+/* Positional parameters don't accept underscores. */
+param  \${decdigit}+
+param_junk \${decdigit}+{ident_start}

scan.l, psqlscan.l and pgc.l are the three files impacted, so that's
good to me.

> Patch 0002 replaces atol with pg_strtoint32_safe in the backend parser
> and strtoint in ECPG.  This fixes overflows like:
> 
> => PREPARE p1 AS SELECT $4294967297;  -- same as $1
> PREPARE
>
> It now returns this error:
> 
> => PREPARE p1 AS SELECT $4294967297;
> ERROR:  parameter too large at or near $4294967297

This one is a much older problem, though.  What you are doing is an
improvement, still I don't see a huge point in backpatching that based
on the lack of complaints with these overflows in the yyac paths.

+if (errno == ERANGE)
+mmfatal(PARSE_ERROR, "parameter too large"); 

Knowong that this is working on decdigits, an ERANGE check should be
enough, indeed.
--
Michael


signature.asc
Description: PGP signature

Re: Underscore in positional parameters?

[Peter Eisentraut]

On 14.05.24 18:07, Erik Wienhold wrote:

Patch 0001 changes rules param and param_junk to only accept digits 0-9.


I have committed this patch to PG16 and master.

I was a little bit on the fence about what the behavior should be, but I 
checked Perl for comparison:


print 1000;   # ok
print 1_000;  # ok
print $1000;  # ok
print $1_000; # error

So this seems alright.


Patch 0002 replaces atol with pg_strtoint32_safe in the backend parser
and strtoint in ECPG.  This fixes overflows like:


Seems like a good idea, but as was said, this is an older issue, so 
let's look at that separately.

Re: Underscore in positional parameters?

[Michael Paquier]

On Wed, May 15, 2024 at 01:59:36PM +0200, Peter Eisentraut wrote:
> On 14.05.24 18:07, Erik Wienhold wrote:
>> Patch 0002 replaces atol with pg_strtoint32_safe in the backend parser
>> and strtoint in ECPG.  This fixes overflows like:
> 
> Seems like a good idea, but as was said, this is an older issue, so let's
> look at that separately.

Hmm, yeah.  I would be really tempted to fix that now.

Now, it has been this way for ages, and with my RMT hat on (aka I need
to show the example), I'd suggest to wait for when the v18 branch
opens as there is no urgency.  I'm OK to apply it myself at the end,
the patch is a good idea.
--
Michael


signature.asc
Description: PGP signature

Re: Underscore in positional parameters?

[Peter Eisentraut]

On 16.05.24 01:11, Michael Paquier wrote:

On Wed, May 15, 2024 at 01:59:36PM +0200, Peter Eisentraut wrote:

On 14.05.24 18:07, Erik Wienhold wrote:

Patch 0002 replaces atol with pg_strtoint32_safe in the backend parser
and strtoint in ECPG.  This fixes overflows like:


Seems like a good idea, but as was said, this is an older issue, so let's
look at that separately.


Hmm, yeah.  I would be really tempted to fix that now.

Now, it has been this way for ages, and with my RMT hat on (aka I need
to show the example), I'd suggest to wait for when the v18 branch
opens as there is no urgency.  I'm OK to apply it myself at the end,
the patch is a good idea.


On this specific patch, maybe reword "parameter too large" to "parameter 
number too large".


Also, I was bemused by the use of atol(), which is notoriously 
unportable (sizeof(long)).  So I poked around and found more places that 
might need fixing.  I'm attaching a patch here with annotations too look 
at later.
From 2d3ad223b1f9b7bb5bebc6b6ef8cbba0d1c0022b Mon Sep 17 00:00:00 2001
From: Peter Eisentraut 
Date: Thu, 16 May 2024 08:36:21 +0200
Subject: [PATCH] WIP: atol() investigation

---
 src/backend/parser/scan.l| 2 +-
 src/bin/pg_basebackup/pg_basebackup.c| 2 +-
 src/bin/pg_basebackup/streamutil.c   | 2 +-
 src/bin/pg_rewind/libpq_source.c | 2 +-
 src/interfaces/ecpg/ecpglib/execute.c| 2 +-
 src/interfaces/ecpg/preproc/ecpg.trailer | 2 +-
 src/interfaces/ecpg/preproc/pgc.l| 2 +-
 7 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index b499975e9c4..383d3f2d39a 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -993,7 +993,7 @@ other   .
 
 {param}{
SET_YYLLOC();
-   yylval->ival = atol(yytext + 1);
+   yylval->ival = atol(yytext + 1); // 
FIXME with overflow check
return PARAM;
}
 {param_junk}   {
diff --git a/src/bin/pg_basebackup/pg_basebackup.c 
b/src/bin/pg_basebackup/pg_basebackup.c
index 8f3dd04fd22..4ebc5e3b2b8 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -2056,7 +2056,7 @@ BaseBackup(char *compression_algorithm, char 
*compression_detail,
tablespacecount = PQntuples(res);
for (i = 0; i < PQntuples(res); i++)
{
-   totalsize_kb += atol(PQgetvalue(res, i, 2));
+   totalsize_kb += atoll(PQgetvalue(res, i, 2)); // FIXME: atol() 
might truncate if sizeof(long)==4
 
/*
 * Verify tablespace directories are empty. Don't bother with 
the
diff --git a/src/bin/pg_basebackup/streamutil.c 
b/src/bin/pg_basebackup/streamutil.c
index d0efd8600ca..1d303d16ef5 100644
--- a/src/bin/pg_basebackup/streamutil.c
+++ b/src/bin/pg_basebackup/streamutil.c
@@ -631,7 +631,7 @@ GetSlotInformation(PGconn *conn, const char *slot_name,
 
/* current TLI */
if (!PQgetisnull(res, 0, 2))
-   tli_loc = (TimeLineID) atol(PQgetvalue(res, 0, 2));
+   tli_loc = (TimeLineID) atol(PQgetvalue(res, 0, 2)); // FIXME: 
use strtoul()
 
PQclear(res);
 
diff --git a/src/bin/pg_rewind/libpq_source.c b/src/bin/pg_rewind/libpq_source.c
index 7d898c3b501..618b175dcc4 100644
--- a/src/bin/pg_rewind/libpq_source.c
+++ b/src/bin/pg_rewind/libpq_source.c
@@ -294,7 +294,7 @@ libpq_traverse_files(rewind_source *source, 
process_file_callback_t callback)
}
 
path = PQgetvalue(res, i, 0);
-   filesize = atol(PQgetvalue(res, i, 1));
+   filesize = atoll(PQgetvalue(res, i, 1)); // FIXME: atol() might 
truncate
isdir = (strcmp(PQgetvalue(res, i, 2), "t") == 0);
link_target = PQgetvalue(res, i, 3);
 
diff --git a/src/interfaces/ecpg/ecpglib/execute.c 
b/src/interfaces/ecpg/ecpglib/execute.c
index 04d0b40c537..c578c21cf66 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -278,7 +278,7 @@ ecpg_is_type_an_array(int type, const struct statement 
*stmt, const struct varia
isarray = ECPG_ARRAY_NONE;
else
{
-   isarray = (atol((char *) PQgetvalue(query, 0, 0)) == 
-1) ? ECPG_ARRAY_ARRAY : ECPG_ARRAY_VECTOR;
+   isarray = (atoi((char *) PQgetvalue(query, 0, 0)) == 
-1) ? ECPG_ARRAY_ARRAY : ECPG_ARRAY_VECTOR;
if (ecpg_dynamic_type(type) == SQL3_CHARACTER ||
ecpg_dynamic_type(type) == 
SQL3_CHARACTER_VARYING)
{
diff --git a/src/interfaces/ecpg/preproc/ecpg.trailer 
b/src/interfaces/ecpg/preproc/ecpg.trailer
index b2aa44f36dd..8ac1c5c9eda 100644
--- a/src/interfaces/ecpg/prep

Re: Underscore in positional parameters?

[Michael Paquier]

On Thu, May 16, 2024 at 08:41:11AM +0200, Peter Eisentraut wrote:
> On this specific patch, maybe reword "parameter too large" to "parameter
> number too large".

WFM here.

> Also, I was bemused by the use of atol(), which is notoriously unportable
> (sizeof(long)).  So I poked around and found more places that might need
> fixing.  I'm attaching a patch here with annotations too look at later.

Yeah atoXX calls have been funky in the tree for some time.  This
reminds this thread, somewhat:
https://www.postgresql.org/message-id/CALAY4q8be6Qw_2J%3DzOp_v1X-zfMBzvVMkAfmMYv%3DUkr%3D2hPcFQ%40mail.gmail.com

The issue is also that there is no "safe" parsing alternative for 64b
integers in the frontend (as you know long is 32b in Windows, which is
why I'd encourage ripping it out as much as we can).  This may be
better as a complementary of strtoint() in src/common/string.c.  Note
as well strtoint64() in pgbench.c.  I think I have a patch lying
around, actually.. 
--
Michael


signature.asc
Description: PGP signature

Re: Underscore in positional parameters?

[Erik Wienhold]

On 2024-05-17 02:06 +0200, Michael Paquier wrote:
> On Thu, May 16, 2024 at 08:41:11AM +0200, Peter Eisentraut wrote:
> > On this specific patch, maybe reword "parameter too large" to "parameter
> > number too large".
> 
> WFM here.

Done in v3.

I noticed this compiler warning with my previous patch:

scan.l:997:41: warning: ISO C90 forbids mixed declarations and code 
[-Wdeclaration-after-statement]
  997 | ErrorSaveContext escontext 
= {T_ErrorSaveContext};
  | ^~~~

I thought that I had to factor this out into a function similar to
process_integer_literal (which also uses ErrorSaveContext).  But moving
that declaration to the start of the {param} action was enough in the
end.

While trying out the refactoring, I noticed two small things that can be
fixed as well in scan.l:

* Prototype and definition of addunicode do not match.  The prototype
  uses yyscan_t while the definition uses core_yyscan_t.

* Parameter base of process_integer_literal is unused.

But those should be one a separate thread, right, even for minor fixes?

-- 
Erik
>From d3ad2971dcb8ab15fafe1a5c69a15e5994eac76d Mon Sep 17 00:00:00 2001
From: Erik Wienhold 
Date: Tue, 14 May 2024 14:12:11 +0200
Subject: [PATCH v3 1/3] Fix overflow in parsing of positional parameter

Replace atol with pg_strtoint32_safe in the backend parser and with
strtoint in ECPG to reject overflows when parsing the number of a
positional parameter.  With atol from glibc, parameters $2147483648 and
$4294967297 turn into $-2147483648 and $1, respectively.
---
 src/backend/parser/scan.l| 8 +++-
 src/interfaces/ecpg/preproc/pgc.l| 5 -
 src/test/regress/expected/numerology.out | 4 
 src/test/regress/sql/numerology.sql  | 1 +
 4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index 9b33fb8d72..436cc64f07 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -992,8 +992,14 @@ other			.
 }
 
 {param}			{
+	ErrorSaveContext escontext = {T_ErrorSaveContext};
+	int32		val;
+
 	SET_YYLLOC();
-	yylval->ival = atol(yytext + 1);
+	val = pg_strtoint32_safe(yytext + 1, (Node *) &escontext);
+	if (escontext.error_occurred)
+		yyerror("parameter number too large");
+	yylval->ival = val;
 	return PARAM;
 }
 {param_junk}	{
diff --git a/src/interfaces/ecpg/preproc/pgc.l b/src/interfaces/ecpg/preproc/pgc.l
index d117cafce6..7122375d72 100644
--- a/src/interfaces/ecpg/preproc/pgc.l
+++ b/src/interfaces/ecpg/preproc/pgc.l
@@ -938,7 +938,10 @@ cppline			{space}*#([^i][A-Za-z]*|{if}|{ifdef}|{ifndef}|{import})((\/\*[^*/]*\*+
 }
 
 {param}			{
-	base_yylval.ival = atol(yytext+1);
+	errno = 0;
+	base_yylval.ival = strtoint(yytext+1, NULL, 10);
+	if (errno == ERANGE)
+		mmfatal(PARSE_ERROR, "parameter number too large");
 	return PARAM;
 }
 {param_junk}	{
diff --git a/src/test/regress/expected/numerology.out b/src/test/regress/expected/numerology.out
index c8196d2c85..5bac05c3b3 100644
--- a/src/test/regress/expected/numerology.out
+++ b/src/test/regress/expected/numerology.out
@@ -206,6 +206,10 @@ PREPARE p1 AS SELECT $1a;
 ERROR:  trailing junk after parameter at or near "$1a"
 LINE 1: PREPARE p1 AS SELECT $1a;
  ^
+PREPARE p1 AS SELECT $2147483648;
+ERROR:  parameter number too large at or near "$2147483648"
+LINE 1: PREPARE p1 AS SELECT $2147483648;
+ ^
 SELECT 0b;
 ERROR:  invalid binary integer at or near "0b"
 LINE 1: SELECT 0b;
diff --git a/src/test/regress/sql/numerology.sql b/src/test/regress/sql/numerology.sql
index 3f0ec34ecf..6722a09c5f 100644
--- a/src/test/regress/sql/numerology.sql
+++ b/src/test/regress/sql/numerology.sql
@@ -52,6 +52,7 @@ SELECT 0.0e1a;
 SELECT 0.0e;
 SELECT 0.0e+a;
 PREPARE p1 AS SELECT $1a;
+PREPARE p1 AS SELECT $2147483648;
 
 SELECT 0b;
 SELECT 1b;
-- 
2.45.1

Re: Underscore in positional parameters?

[Alexander Lakhin]

Hello Erik,

18.05.2024 04:31, Erik Wienhold wrote:

On 2024-05-17 02:06 +0200, Michael Paquier wrote:

On Thu, May 16, 2024 at 08:41:11AM +0200, Peter Eisentraut wrote:

On this specific patch, maybe reword "parameter too large" to "parameter
number too large".

WFM here.

Done in v3.


Thank you for working on this!

I encountered anomalies that you address with this patch too.
And I can confirm that it fixes most cases, but there is another one:
SELECT $3 \bind 'foo' \g
ERROR:  invalid memory alloc request size 12

Maybe you would find this worth fixing as well.

Best regards,
Alexander

Re: Underscore in positional parameters?

[Erik Wienhold]

On 2024-05-19 07:00 +0200, Alexander Lakhin wrote:
> I encountered anomalies that you address with this patch too.
> And I can confirm that it fixes most cases, but there is another one:
> SELECT $3 \bind 'foo' \g
> ERROR:  invalid memory alloc request size 12
> 
> Maybe you would find this worth fixing as well.

Yes, that error message is not great.  In variable_paramref_hook we
check paramno > INT_MAX/sizeof(Oid) when in fact MaxAllocSize/sizeof(Oid)
is the more appropriate limit to avoid that unspecific alloc size error.

Fixed in v4 with a separate patch because it's unrelated to the param
number parsing.  But it fits nicely into the broader issue on the upper
limit for param numbers.  Note that $268435455 is still the largest
possible param number ((2^30-1)/4) and that we just return a more
user-friendly error message for params beyond that limit.

-- 
Erik
>From 5382f725ac1ff99b5830e8ca04613467660afaa2 Mon Sep 17 00:00:00 2001
From: Erik Wienhold 
Date: Tue, 14 May 2024 14:12:11 +0200
Subject: [PATCH v4 1/2] Fix overflow in parsing of positional parameter

Replace atol with pg_strtoint32_safe in the backend parser and with
strtoint in ECPG to reject overflows when parsing the number of a
positional parameter.  With atol from glibc, parameters $2147483648 and
$4294967297 turn into $-2147483648 and $1, respectively.
---
 src/backend/parser/scan.l| 8 +++-
 src/interfaces/ecpg/preproc/pgc.l| 5 -
 src/test/regress/expected/numerology.out | 4 
 src/test/regress/sql/numerology.sql  | 1 +
 4 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/src/backend/parser/scan.l b/src/backend/parser/scan.l
index 9b33fb8d72..436cc64f07 100644
--- a/src/backend/parser/scan.l
+++ b/src/backend/parser/scan.l
@@ -992,8 +992,14 @@ other			.
 }
 
 {param}			{
+	ErrorSaveContext escontext = {T_ErrorSaveContext};
+	int32		val;
+
 	SET_YYLLOC();
-	yylval->ival = atol(yytext + 1);
+	val = pg_strtoint32_safe(yytext + 1, (Node *) &escontext);
+	if (escontext.error_occurred)
+		yyerror("parameter number too large");
+	yylval->ival = val;
 	return PARAM;
 }
 {param_junk}	{
diff --git a/src/interfaces/ecpg/preproc/pgc.l b/src/interfaces/ecpg/preproc/pgc.l
index d117cafce6..7122375d72 100644
--- a/src/interfaces/ecpg/preproc/pgc.l
+++ b/src/interfaces/ecpg/preproc/pgc.l
@@ -938,7 +938,10 @@ cppline			{space}*#([^i][A-Za-z]*|{if}|{ifdef}|{ifndef}|{import})((\/\*[^*/]*\*+
 }
 
 {param}			{
-	base_yylval.ival = atol(yytext+1);
+	errno = 0;
+	base_yylval.ival = strtoint(yytext+1, NULL, 10);
+	if (errno == ERANGE)
+		mmfatal(PARSE_ERROR, "parameter number too large");
 	return PARAM;
 }
 {param_junk}	{
diff --git a/src/test/regress/expected/numerology.out b/src/test/regress/expected/numerology.out
index c8196d2c85..5bac05c3b3 100644
--- a/src/test/regress/expected/numerology.out
+++ b/src/test/regress/expected/numerology.out
@@ -206,6 +206,10 @@ PREPARE p1 AS SELECT $1a;
 ERROR:  trailing junk after parameter at or near "$1a"
 LINE 1: PREPARE p1 AS SELECT $1a;
  ^
+PREPARE p1 AS SELECT $2147483648;
+ERROR:  parameter number too large at or near "$2147483648"
+LINE 1: PREPARE p1 AS SELECT $2147483648;
+ ^
 SELECT 0b;
 ERROR:  invalid binary integer at or near "0b"
 LINE 1: SELECT 0b;
diff --git a/src/test/regress/sql/numerology.sql b/src/test/regress/sql/numerology.sql
index 3f0ec34ecf..6722a09c5f 100644
--- a/src/test/regress/sql/numerology.sql
+++ b/src/test/regress/sql/numerology.sql
@@ -52,6 +52,7 @@ SELECT 0.0e1a;
 SELECT 0.0e;
 SELECT 0.0e+a;
 PREPARE p1 AS SELECT $1a;
+PREPARE p1 AS SELECT $2147483648;
 
 SELECT 0b;
 SELECT 1b;
-- 
2.45.1

>From 9ae52b4a1949a0498dc75aba8fdd72927e6bd93d Mon Sep 17 00:00:00 2001
From: Erik Wienhold 
Date: Sun, 19 May 2024 15:29:18 +0200
Subject: [PATCH v4 2/2] Limit max parameter number with MaxAllocSize

MaxAllocSize puts an upper bound on the largest possible parameter
number ($268435455).  Use that limit instead of MAX_INT to report that
no parameters exist beyond that point instead of reporting an error
about the maximum allocation size being exceeded.
---
 src/backend/parser/parse_param.c  | 3 ++-
 src/test/regress/expected/prepare.out | 5 +
 src/test/regress/sql/prepare.sql  | 3 +++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/src/backend/parser/parse_param.c b/src/backend/parser/parse_param.c
index dbf1a7dff0..b617591ef6 100644
--- a/src/backend/parser/parse_param.c
+++ b/src/backend/parser/parse_param.c
@@ -31,6 +31,7 @@
 #include "parser/parse_param.h"
 #include "utils/builtins.h"
 #include "utils/lsyscache.h"
+#include "utils/memutils.h"
 
 
 typedef struct FixedParamState
@@ -136,7 +137,7 @@ variable_paramref_hook(ParseState *pstate, ParamRef *pref)
 	Param	   *param;
 
 	/* Check parameter number is in range */
-	if (paramno <= 0 || paramno > INT_MAX / sizeof(Oid))
+

Re: Underscore in positional parameters?

[jian he]

On Sun, May 19, 2024 at 10:43 PM Erik Wienhold  wrote:
>
> On 2024-05-19 07:00 +0200, Alexander Lakhin wrote:
> > I encountered anomalies that you address with this patch too.
> > And I can confirm that it fixes most cases, but there is another one:
> > SELECT $3 \bind 'foo' \g
> > ERROR:  invalid memory alloc request size 12
> >
> > Maybe you would find this worth fixing as well.
>
> Yes, that error message is not great.  In variable_paramref_hook we
> check paramno > INT_MAX/sizeof(Oid) when in fact MaxAllocSize/sizeof(Oid)
> is the more appropriate limit to avoid that unspecific alloc size error.
>
> Fixed in v4 with a separate patch because it's unrelated to the param
> number parsing.  But it fits nicely into the broader issue on the upper
> limit for param numbers.  Note that $268435455 is still the largest
> possible param number ((2^30-1)/4) and that we just return a more
> user-friendly error message for params beyond that limit.
>

hi, one minor issue:

/* Check parameter number is in range */
if (paramno <= 0 || paramno > MaxAllocSize / sizeof(Oid))
ereport(ERROR,
(errcode(ERRCODE_UNDEFINED_PARAMETER),
errmsg("there is no parameter $%d", paramno),
parser_errposition(pstate, pref->location)));

if paramno <= 0 then "there is no parameter $%d" makes sense to me.

but, if paramno > 0 why not just say, we can only allow MaxAllocSize /
sizeof(Oid) number of parameters.

Re: Underscore in positional parameters?

[Erik Wienhold]

On 2024-05-20 03:26 +0200, jian he wrote:
> On Sun, May 19, 2024 at 10:43 PM Erik Wienhold  wrote:
> >
> > On 2024-05-19 07:00 +0200, Alexander Lakhin wrote:
> > > I encountered anomalies that you address with this patch too.
> > > And I can confirm that it fixes most cases, but there is another one:
> > > SELECT $3 \bind 'foo' \g
> > > ERROR:  invalid memory alloc request size 12
> > >
> > > Maybe you would find this worth fixing as well.
> >
> > Yes, that error message is not great.  In variable_paramref_hook we
> > check paramno > INT_MAX/sizeof(Oid) when in fact MaxAllocSize/sizeof(Oid)
> > is the more appropriate limit to avoid that unspecific alloc size error.
> >
> > Fixed in v4 with a separate patch because it's unrelated to the param
> > number parsing.  But it fits nicely into the broader issue on the upper
> > limit for param numbers.  Note that $268435455 is still the largest
> > possible param number ((2^30-1)/4) and that we just return a more
> > user-friendly error message for params beyond that limit.
> >
> 
> hi, one minor issue:
> 
> /* Check parameter number is in range */
> if (paramno <= 0 || paramno > MaxAllocSize / sizeof(Oid))
> ereport(ERROR,
> (errcode(ERRCODE_UNDEFINED_PARAMETER),
> errmsg("there is no parameter $%d", paramno),
> parser_errposition(pstate, pref->location)));
> 
> if paramno <= 0 then "there is no parameter $%d" makes sense to me.
> 
> but, if paramno > 0 why not just say, we can only allow MaxAllocSize /
> sizeof(Oid) number of parameters.

Yes, it makes sense to show the upper bound.  How about a hint such as
"Valid parameters range from $%d to $%d."?

-- 
Erik

Re: Underscore in positional parameters?

[Tom Lane]

Erik Wienhold  writes:
> On 2024-05-20 03:26 +0200, jian he wrote:
>> /* Check parameter number is in range */
>> if (paramno <= 0 || paramno > MaxAllocSize / sizeof(Oid))
>> ereport(ERROR, ...

> Yes, it makes sense to show the upper bound.  How about a hint such as
> "Valid parameters range from $%d to $%d."?

I kind of feel like this upper bound is ridiculous.  In what scenario
is parameter 25000 not a mistake, if not indeed somebody trying
to break the system?

The "Bind" protocol message only allows an int16 parameter count,
so rejecting parameter numbers above 32K would make sense to me.

regards, tom lane

Re: Underscore in positional parameters?

[Erik Wienhold]

On 2024-05-20 05:02 +0200, Tom Lane wrote:
> Erik Wienhold  writes:
> > On 2024-05-20 03:26 +0200, jian he wrote:
> >> /* Check parameter number is in range */
> >> if (paramno <= 0 || paramno > MaxAllocSize / sizeof(Oid))
> >> ereport(ERROR, ...
> 
> > Yes, it makes sense to show the upper bound.  How about a hint such as
> > "Valid parameters range from $%d to $%d."?
> 
> I kind of feel like this upper bound is ridiculous.  In what scenario
> is parameter 25000 not a mistake, if not indeed somebody trying
> to break the system?
> 
> The "Bind" protocol message only allows an int16 parameter count,
> so rejecting parameter numbers above 32K would make sense to me.

Agree.  I was already wondering upthread why someone would use that many
parameters.

Out of curiosity, I checked if there might be an even lower limit.  And
indeed, max_stack_depth puts a limit due to some recursive evaluation:

ERROR:  stack depth limit exceeded
HINT:  Increase the configuration parameter "max_stack_depth" (currently 
2048kB), after ensuring the platform's stack depth limit is adequate.

Attached is the stacktrace for EXECUTE on HEAD (I snipped most of the
recursive frames).

Running \bind, PREPARE, and EXECUTE with following number of parameters
works as expected, although the number varies between releases which is
not ideal IMO.  The commands hit the stack depth limit for #Params+1.

VersionCommand  #Params
-  ---  ---
HEAD (18cbed13d5)  \bind4365
HEAD (18cbed13d5)  PREPARE  8182
HEAD (18cbed13d5)  EXECUTE  4363
16.2   \bind3968
16.2   PREPARE  6889
16.2   EXECUTE  3966

Those are already pretty large numbers in my view (compared to the 100
parameters that we accept at most for functions).  And I guess nobody
complained about those limits yet, or they just increased
max_stack_depth.

The Python script to generate the test scripts:

import sys
n_params = 1 << 16
if len(sys.argv) > 1:
n_params = min(n_params, int(sys.argv[1]))
params = '+'.join(f'${i+1}::int' for i in range(n_params))
bind_vals = ' '.join('1' for _ in range(n_params))
exec_vals = ','.join('1' for _ in range(n_params))
print(fr"SELECT {params} \bind {bind_vals} \g")
print(f"PREPARE p AS SELECT {params};")
print(f"EXECUTE p ({exec_vals});")

-- 
Erik
Breakpoint 1, errfinish (filename=0x6441fddaec2e "postgres.c", lineno=3535, 
funcname=0x6441fdef87a0 <__func__.14> "check_stack_depth") at elog.c:479
479 ErrorData  *edata = &errordata[errordata_stack_depth];
#0  errfinish (filename=0x6441fddaec2e "postgres.c", lineno=3535, 
funcname=0x6441fdef87a0 <__func__.14> "check_stack_depth") at elog.c:479
#1  0x6441fd898173 in check_stack_depth () at postgres.c:3535
#2  0x6441fda5a9a2 in ExecInitExprRec (node=node@entry=0x6442373be5b0, 
state=state@entry=0x644236db7e90, resv=resv@entry=0x644236db7e98, 
resnull=resnull@entry=0x644236db7e95) at execExpr.c:899
#3  0x6441fda5dc12 in ExecInitExpr (node=node@entry=0x6442373be5b0, 
parent=parent@entry=0x0) at execExpr.c:153
#4  0x6441fdb52f58 in evaluate_expr (expr=0x6442373be5b0, 
result_type=result_type@entry=23, result_typmod=result_typmod@entry=-1, 
result_collation=result_collation@entry=0) at clauses.c:4987
#5  0x6441fdb53181 in evaluate_function (func_tuple=0x71b8821811c0, 
funcid=177, result_type=23, result_typmod=-1, result_collid=0, input_collid=0, 
args=0x6442373be520, funcvariadic=false, context=0x7ffd36b93cc0) at 
clauses.c:4503
#6  simplify_function (funcid=177, result_type=23, 
result_typmod=result_typmod@entry=-1, result_collid=0, input_collid=0, 
args_p=args_p@entry=0x7ffd36994940, funcvariadic=false, process_args=true, 
allow_non_const=true, context=0x7ffd36b93cc0) at clauses.c:4092
#7  0x6441fdb54624 in eval_const_expressions_mutator (node=, 
context=0x7ffd36b93cc0) at clauses.c:2639
#8  0x6441fdacdf3a in expression_tree_mutator_impl (node=0x644236d87398, 
mutator=mutator@entry=0x6441fdb53a10 , 
context=context@entry=0x7ffd36b93cc0) at nodeFuncs.c:3554
#9  0x6441fdb53781 in simplify_function (funcid=177, result_type=23, 
result_typmod=result_typmod@entry=-1, result_collid=0, input_collid=0, 
args_p=args_p@entry=0x7ffd36994b20, funcvariadic=false, process_args=true, 
allow_non_const=true, context=0x7ffd36b93cc0) at clauses.c:4083
#10 0x6441fdb54624 in eval_const_expressions_mutator (node=, 
context=0x7ffd36b93cc0) at clauses.c:2639
#11 0x6441fdacdf3a in expression_tree_mutator_impl (node=0x644236d87308, 
mutator=mutator@entry=0x6441fdb53a10 , 
context=context@entry=0x7ffd36b93cc0) at nodeFuncs.c:3554
#12 0x6441fdb53781 in simplify_function (funcid=177, result_type=23, 
result_typmod=result_typmod@entry=-1, result_collid=0, input_collid=0, 
args_p=args_p@entry=0x7ffd36994d00, funcvariadic=false, process_args=true, 
allow_non_const=true, context=0x7ffd36b93cc0) at clauses.c:4083
#13 0x6441fdb54624 in eval_const_expres

Re: Underscore in positional parameters?

[Peter Eisentraut]

On 19.05.24 16:43, Erik Wienhold wrote:

On 2024-05-19 07:00 +0200, Alexander Lakhin wrote:

I encountered anomalies that you address with this patch too.
And I can confirm that it fixes most cases, but there is another one:
SELECT $3 \bind 'foo' \g
ERROR:  invalid memory alloc request size 12

Maybe you would find this worth fixing as well.


Yes, that error message is not great.  In variable_paramref_hook we
check paramno > INT_MAX/sizeof(Oid) when in fact MaxAllocSize/sizeof(Oid)
is the more appropriate limit to avoid that unspecific alloc size error.

Fixed in v4 with a separate patch because it's unrelated to the param
number parsing.  But it fits nicely into the broader issue on the upper
limit for param numbers.  Note that $268435455 is still the largest
possible param number ((2^30-1)/4) and that we just return a more
user-friendly error message for params beyond that limit.


I have committed your two v4 patches.

I made a small adjustment in 0001: I changed the ecpg part to also store 
the result from strtoint() into a local variable before checking for 
error, like you had done in the scan.l part.  I think this is a bit 
better style.  In 0002 you had a typo in the commit message: MAX_INT 
instead of INT_MAX.

Re: Underscore in positional parameters?

[Peter Eisentraut]

On 02.07.24 10:14, Peter Eisentraut wrote:

On 19.05.24 16:43, Erik Wienhold wrote:

On 2024-05-19 07:00 +0200, Alexander Lakhin wrote:

I encountered anomalies that you address with this patch too.
And I can confirm that it fixes most cases, but there is another one:
SELECT $3 \bind 'foo' \g
ERROR:  invalid memory alloc request size 12

Maybe you would find this worth fixing as well.


Yes, that error message is not great.  In variable_paramref_hook we
check paramno > INT_MAX/sizeof(Oid) when in fact MaxAllocSize/sizeof(Oid)
is the more appropriate limit to avoid that unspecific alloc size error.

Fixed in v4 with a separate patch because it's unrelated to the param
number parsing.  But it fits nicely into the broader issue on the upper
limit for param numbers.  Note that $268435455 is still the largest
possible param number ((2^30-1)/4) and that we just return a more
user-friendly error message for params beyond that limit.


I have committed your two v4 patches.

I made a small adjustment in 0001: I changed the ecpg part to also store 
the result from strtoint() into a local variable before checking for 
error, like you had done in the scan.l part.  I think this is a bit 
better style.  In 0002 you had a typo in the commit message: MAX_INT 
instead of INT_MAX.


I had to revert the test case from the 0002 patch.  It ended up running 
some build farm machines out of memory.

Re: Underscore in positional parameters?

[Erik Wienhold]

On 2024-07-02 10:14 +0200, Peter Eisentraut wrote:
> On 19.05.24 16:43, Erik Wienhold wrote:
> > On 2024-05-19 07:00 +0200, Alexander Lakhin wrote:
> > > I encountered anomalies that you address with this patch too.
> > > And I can confirm that it fixes most cases, but there is another one:
> > > SELECT $3 \bind 'foo' \g
> > > ERROR:  invalid memory alloc request size 12
> > > 
> > > Maybe you would find this worth fixing as well.
> > 
> > Yes, that error message is not great.  In variable_paramref_hook we
> > check paramno > INT_MAX/sizeof(Oid) when in fact MaxAllocSize/sizeof(Oid)
> > is the more appropriate limit to avoid that unspecific alloc size error.
> > 
> > Fixed in v4 with a separate patch because it's unrelated to the param
> > number parsing.  But it fits nicely into the broader issue on the upper
> > limit for param numbers.  Note that $268435455 is still the largest
> > possible param number ((2^30-1)/4) and that we just return a more
> > user-friendly error message for params beyond that limit.
> 
> I have committed your two v4 patches.
> 
> I made a small adjustment in 0001: I changed the ecpg part to also store the
> result from strtoint() into a local variable before checking for error, like
> you had done in the scan.l part.  I think this is a bit better style.  In
> 0002 you had a typo in the commit message: MAX_INT instead of INT_MAX.

Thanks Peter!

-- 
Erik

Re: Underscore in positional parameters?

[Erik Wienhold]

On 2024-07-02 10:45 +0200, Peter Eisentraut wrote:
> On 02.07.24 10:14, Peter Eisentraut wrote:
> > I have committed your two v4 patches.
> 
> I had to revert the test case from the 0002 patch.  It ended up running some
> build farm machines out of memory.

dhole, morepork, and schnauzer.  For example, schnauzer[1]:

> diff -U3 
> /home/pgbf/buildroot/HEAD/pgsql.build/src/test/regress/expected/prepare.out 
> /home/pgbf/buildroot/HEAD/pgsql.build/src/test/regress/results/prepare.out
> --- 
> /home/pgbf/buildroot/HEAD/pgsql.build/src/test/regress/expected/prepare.out   
> Tue Jul  2 10:31:34 2024
> +++ 
> /home/pgbf/buildroot/HEAD/pgsql.build/src/test/regress/results/prepare.out
> Tue Jul  2 10:33:15 2024
> @@ -186,9 +186,8 @@
> 
>  -- max parameter number and one above
>  PREPARE q9 AS SELECT $268435455, $268435456;
> -ERROR:  there is no parameter $268435456
> -LINE 1: PREPARE q9 AS SELECT $268435455, $268435456;
> - ^
> +ERROR:  out of memory
> +DETAIL:  Failed on request of size 1073741820 in memory context 
> "PortalContext".
>  -- test DEALLOCATE ALL;
>  DEALLOCATE ALL;
>  SELECT name, statement, parameter_types FROM pg_prepared_statements

That means paramno is less than MaxAllocSize/sizeof(Oid) if it tries to
allocate memory.  MaxAllocSize is always 0x3fff.  Is sizeof(Oid)
less than 4 on those machines?

[1] 
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=schnauzer&dt=2024-07-02%2008%3A31%3A34

-- 
Erik

Re: Underscore in positional parameters?

[Tom Lane]

Peter Eisentraut  writes:
> I had to revert the test case from the 0002 patch.  It ended up running 
> some build farm machines out of memory.

That ties into what I said upthread: why are we involving MaxAllocSize
in this at all?  The maximum parameter number you can actually use in
extended queries is 65535 (because 16-bit fields), and I can't see a
good reason to permit more.

regards, tom lane

Re: Underscore in positional parameters?

[Tom Lane]

Erik Wienhold  writes:
> On 2024-07-02 10:45 +0200, Peter Eisentraut wrote:
>> I had to revert the test case from the 0002 patch.  It ended up running some
>> build farm machines out of memory.

>> +ERROR:  out of memory
>> +DETAIL:  Failed on request of size 1073741820 in memory context 
>> "PortalContext".

> That means paramno is less than MaxAllocSize/sizeof(Oid) if it tries to
> allocate memory.  MaxAllocSize is always 0x3fff.  Is sizeof(Oid)
> less than 4 on those machines?

No.  Y'know, it's not really *that* astonishing for a machine to not
have a spare 1GB of RAM available on-demand.  This test would
certainly have failed on our 32-bit animals, although it doesn't
look like any of them had gotten to it yet.

regards, tom lane

Re: Underscore in positional parameters?

[Peter Eisentraut]

On 02.07.24 16:14, Tom Lane wrote:

Peter Eisentraut  writes:

I had to revert the test case from the 0002 patch.  It ended up running
some build farm machines out of memory.


That ties into what I said upthread: why are we involving MaxAllocSize
in this at all?  The maximum parameter number you can actually use in
extended queries is 65535 (because 16-bit fields), and I can't see a
good reason to permit more.


There are arguably a few things that could be done in this area of code 
to improve it, like consistently using int16 and strtoint16 and so on 
for parameter numbers.  But that's a different project.


The change here was merely to an existing check that apparently wanted 
to avoid some kind of excessive memory allocation but did so 
ineffectively by checking against INT_MAX, which had nothing to do with 
how the memory allocation checking actually works.  The fixed code now 
avoids the error for "invalid memory alloc request size", but of course 
it can still fail if the OS does not have enough memory.

Re: Underscore in positional parameters?

[Erik Wienhold]

On 2024-07-02 16:21 +0200, Tom Lane wrote:
> Erik Wienhold  writes:
> > On 2024-07-02 10:45 +0200, Peter Eisentraut wrote:
> >> I had to revert the test case from the 0002 patch.  It ended up running 
> >> some
> >> build farm machines out of memory.
> 
> >> +ERROR:  out of memory
> >> +DETAIL:  Failed on request of size 1073741820 in memory context 
> >> "PortalContext".
> 
> > That means paramno is less than MaxAllocSize/sizeof(Oid) if it tries to
> > allocate memory.  MaxAllocSize is always 0x3fff.  Is sizeof(Oid)
> > less than 4 on those machines?
> 
> No.  Y'know, it's not really *that* astonishing for a machine to not
> have a spare 1GB of RAM available on-demand.  This test would
> certainly have failed on our 32-bit animals, although it doesn't
> look like any of them had gotten to it yet.

Ah, sorry.  I somehow missed that it allocates memory for each param,
instead of first checking *all* params.  m(

-- 
Erik