Обсуждение: PSQL error: total cell count of XXX exceeded

When I tried to select a big amount of rows, psql complains a error "Cannot add cell to table content: total cell count of 905032704 exceeded."

 We should use long for ncolumns and nrows and give a more obvious error message here.

Any thoughts? or some other hidden reasons?

On Mon, 11 Sept 2023 at 08:51, Hongxu Ma <interma@outlook.com> wrote:
>
> I created a patch to fix it.
> Really appreciate to anyone can help to review it.
> Thanks.

I think "product" as a variable name isn't very descriptive. Let's
call it total_cells (or something similar instead).

Other than that I think it's a good change. content->cellsadded is
also a long, So I agree that I don't think the limit of int cells was
intended here.

On Tue, Sep 12, 2023 at 02:39:55AM +0000, Hongxu Ma wrote:
> Thank you for your advice, Jelte.
> I have refactored my code, please see the attached patch. (and I put
> it into https://commitfest.postgresql.org/45/ for trace)

 {
+       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?
--
Michael

Michael Paquier <michael@paquier.xyz> 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

On 2023-Sep-12, Tom Lane wrote:

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

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)

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> 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.

This bit needs more work:

-    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));

You've made the computation of total_cells reliable, but there's
nothing stopping the subsequent computation of the malloc argument
from overflowing (especially on 32-bit machines).  I think we need
an explicit test along the lines of

    if (total_cells >= SIZE_MAX / sizeof(*content->cells))
        throw error;

(">=" allows not needing to add +1.)

Also, maybe total_cells should be uint64?  We don't want
negative values to pass this test.  Alternatively, add a separate
check that total_cells >= 0.

It should be sufficient to be paranoid about this in printTableInit,
since after that we know the product of ncolumns * nrows isn't
too big.

The rest of this passes an eyeball check.

            regards, tom lane

On 2023-Sep-13, Hongxu Ma wrote:

> After double check, looks `int64` of src/include/c.h is the proper type for it.
> 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/

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> Right.  I made a few more adjustments, including the additional overflow
> check in printTableInit that Tom Lane suggested, and pushed this.

Committed patch LGTM.

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

Yeah, I think the reason it's stayed like that for 25 years is that
nobody's hit the case in practice.  Changing the API would be a bit
troublesome, too, because we don't know if anybody besides psql
uses it.

            regards, tom lane

On 2023-Nov-21, Tom Lane wrote:

> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > Right.  I made a few more adjustments, including the additional overflow
> > check in printTableInit that Tom Lane suggested, and pushed this.
> 
> Committed patch LGTM.

Thanks for looking!

> > It's a bit annoying that the error recovery decision of this code is to
> > exit the process with an error.  [...]
> > TBH though, I've never hit that code in real usage.
> 
> Yeah, I think the reason it's stayed like that for 25 years is that
> nobody's hit the case in practice.  Changing the API would be a bit
> troublesome, too, because we don't know if anybody besides psql
> uses it.

True.

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

Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> On 2023-Nov-21, Tom Lane wrote:
>> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
>>> It's a bit annoying that the error recovery decision of this code is to
>>> exit the process with an error.  [...]
>>> TBH though, I've never hit that code in real usage.

>> Yeah, I think the reason it's stayed like that for 25 years is that
>> nobody's hit the case in practice.  Changing the API would be a bit
>> troublesome, too, because we don't know if anybody besides psql
>> uses it.

> True.

It strikes me that perhaps a workable compromise behavior could be
"report the error to wherever we would have printed the table, and
return normally".  I'm still not excited about doing anything about
it, but ...

            regards, tom lane