Subject: Re: PSQL error: total cell count of XXX exceeded

From: Tom Lane <[email-masked]>
Date: Tue, 12 Sep 2023 00:19:53 -0400

Michael Paquier <[email-masked]> writes:
> On Tue, Sep 12, 2023 at 02:39:55AM +0000, Hongxu Ma wrote:
> +       long total_cells;

> long is 4 bytes on Windows, and 8 bytes basically elsewhere.  So you
> would still have the same problem on Windows, no?

More to the point: what about the multiplication in printTableInit?
The cat's been out of the bag for quite some time before we get to
printTableAddCell.

I'm more than a bit skeptical about trying to do something about this,
simply because this range of query result sizes is far past what is
practical.  The OP clearly hasn't tested his patch on actually
overflowing query results, and I don't care to either.

			regards, tom lane

---

From: Hongxu Ma <[email-masked]>
Date: Wed, 13 Sep 2023 02:22:48 +0000

Thanks for pointing that, I did miss some other "ncols * nrows" places. Uploaded v3 patch to fix them.

As for the Windows, I didn't test it before but I think it should also have the issue (and happens more possible since ​`cellsadded` is also a long type).
My fix idea is simple: define a common long64 type for it.
I referred MSDN: only `LONGLONG` and `LONG64` are 64 bytes. And I assume Postgres should already have a similar type, but only found `typedef long int int64` in src/include/c.h, looks it's not a proper choose.
@Michael Paquier<mailto:[email-masked]>, could you help to give some advices here (which type should be used? or should define a new one?). Thank you very much.

________________________________
From: Tom Lane <[email-masked]>
Sent: Tuesday, September 12, 2023 12:19
To: Michael Paquier <[email-masked]>
Cc: Hongxu Ma <[email-masked]>; Jelte Fennema <[email-masked]>; David G. Johnston <[email-masked]>; PostgreSQL Hackers <[email-masked]>
Subject: Re: PSQL error: total cell count of XXX exceeded

                        regards, tom lane

---

From: Hongxu Ma <[email-masked]>
Date: Wed, 13 Sep 2023 07:31:54 +0000

After double check, looks `int64` of src/include/c.h is the proper type for it.
Uploaded the v4 patch to fix it.
Thanks.

________________________________
From: Hongxu Ma <[email-masked]>
Sent: Wednesday, September 13, 2023 10:22
To: Tom Lane <[email-masked]>; Michael Paquier <[email-masked]>
Cc: PostgreSQL Hackers <[email-masked]>
Subject: Re: PSQL error: total cell count of XXX exceeded

                        regards, tom lane

---

From: Alvaro Herrera <[email-masked]>
Date: Mon, 20 Nov 2023 21:48:35 +0100

> overflowing query results, and I don't care to either.

I think we're bound to hit this limit at some point in the future, and
it seems easy enough to solve.  I propose the attached, which is pretty
much what Hongxu last submitted, with some minor changes.

Having this make a difference requires some 128GB of RAM, so it's not a
piece of cake, but it's an amount that can be reasonably expected to be
physically installed in real machines nowadays.

(I first thought we could just use pg_mul_s32_overflow during
printTableInit and raise an error if that returns true, but that just
postpones the problem.)

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
Subversion to GIT: the shortest path to happiness I've ever heard of
                                                (Alexey Klyukin)
>From 75abe5a485532cbc03db1eade2e1a6099914f98f Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <[email-masked]>
Date: Mon, 20 Nov 2023 17:35:18 +0100
Subject: [PATCH v5] Avoid overflow in fe_utils' printTable API

We were using 32-bit integer arithmetic to compute the total number of
cells, which can overflow when used with large resultsets.  We still
don't allow more than 4 billion rows, but now the total number of cells
can exceed that.

Author: Hongxu Ma <[email-masked]>
Discussion: https://postgr.es/m/[email-masked]
---
 src/fe_utils/print.c         | 22 ++++++++++++++--------
 src/include/fe_utils/print.h |  2 +-
 2 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/src/fe_utils/print.c b/src/fe_utils/print.c
index 7af1ccb6b5..565cbc6d13 100644
--- a/src/fe_utils/print.c
+++ b/src/fe_utils/print.c
@@ -3172,6 +3172,8 @@ void
 printTableInit(printTableContent *const content, const printTableOpt *opt,
 			   const char *title, const int ncolumns, const int nrows)
 {
+	int64		total_cells;
+
 	content->opt = opt;
 	content->title = title;
 	content->ncolumns = ncolumns;
@@ -3179,7 +3181,8 @@ printTableInit(printTableContent *const content, const printTableOpt *opt,
 
 	content->headers = pg_malloc0((ncolumns + 1) * sizeof(*content->headers));
 
-	content->cells = pg_malloc0((ncolumns * nrows + 1) * sizeof(*content->cells));
+	total_cells = (int64) ncolumns * nrows;
+	content->cells = pg_malloc0((total_cells + 1) * sizeof(*content->cells));
 
 	content->cellmustfree = NULL;
 	content->footers = NULL;
@@ -3249,15 +3252,17 @@ void
 printTableAddCell(printTableContent *const content, char *cell,
 				  const bool translate, const bool mustfree)
 {
+	int64		total_cells;
+
 #ifndef ENABLE_NLS
 	(void) translate;			/* unused parameter */
 #endif
 
-	if (content->cellsadded >= content->ncolumns * content->nrows)
+	total_cells = (int64) content->ncolumns * content->nrows;
+	if (content->cellsadded >= total_cells)
 	{
-		fprintf(stderr, _("Cannot add cell to table content: "
-						  "total cell count of %d exceeded.\n"),
-				content->ncolumns * content->nrows);
+		fprintf(stderr, _("Cannot add cell to table content: total cell count of %lld exceeded.\n"),
+				(long long int) total_cells);
 		exit(EXIT_FAILURE);
 	}
 
@@ -3273,7 +3278,7 @@ printTableAddCell(printTableContent *const content, char *cell,
 	{
 		if (content->cellmustfree == NULL)
 			content->cellmustfree =
-				pg_malloc0((content->ncolumns * content->nrows + 1) * sizeof(bool));
+				pg_malloc0((total_cells + 1) * sizeof(bool));
 
 		content->cellmustfree[content->cellsadded] = true;
 	}
@@ -3341,9 +3346,10 @@ printTableCleanup(printTableContent *const content)
 {
 	if (content->cellmustfree)
 	{
-		int			i;
+		int64		total_cells;
 
-		for (i = 0; i < content->nrows * content->ncolumns; i++)
+		total_cells = (int64) content->ncolumns * content->nrows;
+		for (int64 i = 0; i < total_cells; i++)
 		{
 			if (content->cellmustfree[i])
 				free(unconstify(char *, content->cells[i]));
diff --git a/src/include/fe_utils/print.h b/src/include/fe_utils/print.h
index 13ebb00362..a697d0ba8d 100644
--- a/src/include/fe_utils/print.h
+++ b/src/include/fe_utils/print.h
@@ -171,7 +171,7 @@ typedef struct printTableContent
 	const char **cells;			/* NULL-terminated array of cell content
 								 * strings */
 	const char **cell;			/* Pointer to the last added cell */
-	long		cellsadded;		/* Number of cells added this far */
+	int64		cellsadded;		/* Number of cells added this far */
 	bool	   *cellmustfree;	/* true for cells that need to be free()d */
 	printTableFooter *footers;	/* Pointer to the first footer */
 	printTableFooter *footer;	/* Pointer to the last added footer */
-- 
2.39.2

---

From: Alvaro Herrera <[email-masked]>
Date: Tue, 21 Nov 2023 15:33:03 +0100

> Uploaded the v4 patch to fix it.

Right.  I made a few more adjustments, including the additional overflow
check in printTableInit that Tom Lane suggested, and pushed this.

It's a bit annoying that the error recovery decision of this code is to
exit the process with an error.  If somebody were to be interested in a
fun improvement exercise, it may be worth redoing the print.c API so
that it returns errors that psql can report and recover from, instead of
just closing the process.

TBH though, I've never hit that code in real usage.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/