pg.ddx.io pgsql-hackers@postgresql.org mailing list archive
help / color / mirror / Atom feedoverflow bug for inhcounts
6+ messages / 4 participants
[nested] [flat]
Commits from this thread:
2024-10-10 fd64ed60b6 Unbreak overflow test for attinhcount/coninhcount
* overflow bug for inhcounts
@ 2024-10-08 16:11 Alvaro Herrera <alvherre@alvh.no-ip.org>
2024-10-08 16:47 ` Re: overflow bug for inhcounts Tom Lane <tgl@sss.pgh.pa.us>
2024-10-08 17:00 ` Re: overflow bug for inhcounts Andres Freund <andres@anarazel.de>
2024-10-09 02:01 ` Re: overflow bug for inhcounts jian he <jian.universality@gmail.com>
0 siblings, 3 replies; 6+ messages in thread
From: Alvaro Herrera @ 2024-10-08 16:11 UTC (permalink / raw)
To: Pg Hackers <pgsql-hackers@lists.postgresql.org>; +Cc: jian he <jian.universality@gmail.com>
Per a review comment on the not-null constraint patch from Jian He, I
played a bit with the various inhcount variables in the tree, with this
simple test which creates a child table and 2^16+1 parents:
for i in `seq 1 $((2 ** 16+1))`; do echo "create table parent_$i (a int);"; done | psql
(echo "create table child () inherits (" ;
for i in `seq 1 $(( 2**16 ))`; do
echo -n "parent_$i,";
done;
echo "parent_$((2**16 + 1))) ;" ) | psql
(Needs a larger-than-default max_locks_per_transaction). With current
master, this command finishes successfully, but sets attinhcount to 1
for the column:
select attname, attnum, attinhcount from pg_attribute where attrelid = 'child'::regclass and attnum > 0;
attname │ attnum │ attinhcount
─────────┼────────┼─────────────
a │ 1 │ 1
This is because ColumnDef->inhcount is a 32-bit int, but
Form_pg_attribute->attinhcount is int16, so we didn't break the overflow
test for ColumnDef inhcount, but attinhcount has overflowed during
assignment.
At this point we can disinherit the table from a single parent (which
will bring the attinhcount to 0) and then drop the column. This breaks
any query that examines one of the other 2^16 parents,
=# alter table child no inherit parent_1;
ALTER TABLE
=# alter table child drop column a;
ALTER TABLE
=# select * from parent_2;
ERROR: could not find inherited attribute "a" of relation "child"
I suppose the same could happen with CookedConstraint->inhcount, which
is also 32-bit int, but I didn't build an example for that.
From branch master, I propose we change those two members to int16
(ColumnDef->inhcount and CookedConstraint->inhcount) to make those
counters consistently use the same type; and then use
pg_add_s16_overflow() instead of ++ for the increments, as in the
attached patch. With this patch, the child table creation fails as
expected ("too many inheritance parents").
In stable branches, I see two possible approaches: we could use the same
ptach as master (but add another int16 to the struct as padding, to
avoid changing the struct layout), or, less intrusive, we could leave
that alone and instead change the "overflow" after the addition to test
inhcount > PG_INT16_MAX instead of < 0. Or we could leave it all alone.
Oh and actually, we could change all these variables to be unsigned,
since there's no use for negative inhcounts. The patch doesn't do that;
it'd require changing the subtraction paths to use overflow-protected
ops as well.
Opinions?
(I'm not terribly enthused about adding a test that creates a child
table with 2^16 parents, because of the added runtime -- on my machine
the scripts above take about 4 seconds.)
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"Las cosas son buenas o malas segun las hace nuestra opinión" (Lisias)
Attachments:
0001-Unbreak-overflow-test-for-attinhcount-coninhcount.patch (text/x-diff, 6.2K) download
From 45ff2614e85c47b339e04da147dff74995a0540c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvherre@alvh.no-ip.org>
Date: Tue, 8 Oct 2024 16:42:04 +0200
Subject: [PATCH] Unbreak overflow test for attinhcount/coninhcount
We're assigning from ColumnDef->inhcount and CookedConstraint->inhcount,
which are 32-bit wide, so they could overflow. Change the latter two to
using int16 as well to avoid the problem.
Also, modernize the style by using pg_add_s16_overflow for overflow
testing instead.
---
src/backend/catalog/heap.c | 6 ++----
src/backend/catalog/pg_constraint.c | 6 ++++--
src/backend/commands/tablecmds.c | 22 ++++++++++++----------
src/include/catalog/heap.h | 2 +-
src/include/nodes/parsenodes.h | 2 +-
5 files changed, 20 insertions(+), 18 deletions(-)
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 78e59384d1..eb3cb83679 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2624,10 +2624,8 @@ MergeWithExistingConstraint(Relation rel, const char *ccname, Node *expr,
{
if (is_local)
con->conislocal = true;
- else
- con->coninhcount++;
-
- if (con->coninhcount < 0)
+ else if (pg_add_s16_overflow(con->coninhcount, 1,
+ &con->coninhcount))
ereport(ERROR,
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("too many inheritance parents"));
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index 1e2df031a8..c36ea885bf 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -27,6 +27,7 @@
#include "catalog/pg_operator.h"
#include "catalog/pg_type.h"
#include "commands/defrem.h"
+#include "common/int.h"
#include "utils/array.h"
#include "utils/builtins.h"
#include "utils/fmgroids.h"
@@ -849,11 +850,12 @@ ConstraintSetParentConstraint(Oid childConstrId,
childConstrId);
constrForm->conislocal = false;
- constrForm->coninhcount++;
- if (constrForm->coninhcount < 0)
+ if (pg_add_s16_overflow(constrForm->coninhcount, 1,
+ &constrForm->coninhcount))
ereport(ERROR,
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("too many inheritance parents"));
+
constrForm->conparentid = parentConstrId;
CatalogTupleUpdate(constrRel, &tuple->t_self, newtup);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index af8c05b91f..06a76a4b5e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -66,6 +66,7 @@
#include "commands/typecmds.h"
#include "commands/user.h"
#include "commands/vacuum.h"
+#include "common/int.h"
#include "executor/executor.h"
#include "foreign/fdwapi.h"
#include "foreign/foreign.h"
@@ -3044,8 +3045,8 @@ MergeCheckConstraint(List *constraints, const char *name, Node *expr)
if (equal(expr, ccon->expr))
{
/* OK to merge constraint with existing */
- ccon->inhcount++;
- if (ccon->inhcount < 0)
+ if (pg_add_s16_overflow(ccon->inhcount, 1,
+ &ccon->inhcount))
ereport(ERROR,
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("too many inheritance parents"));
@@ -3347,8 +3348,8 @@ MergeInheritedAttribute(List *inh_columns,
* Default and other constraints are handled by the caller.
*/
- prevdef->inhcount++;
- if (prevdef->inhcount < 0)
+ if (pg_add_s16_overflow(prevdef->inhcount, 1,
+ &prevdef->inhcount))
ereport(ERROR,
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("too many inheritance parents"));
@@ -7089,8 +7090,8 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
get_collation_name(childatt->attcollation))));
/* Bump the existing child att's inhcount */
- childatt->attinhcount++;
- if (childatt->attinhcount < 0)
+ if (pg_add_s16_overflow(childatt->attinhcount, 1,
+ &childatt->attinhcount))
ereport(ERROR,
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("too many inheritance parents"));
@@ -15944,8 +15945,8 @@ MergeAttributesIntoExisting(Relation child_rel, Relation parent_rel, bool ispart
* OK, bump the child column's inheritance count. (If we fail
* later on, this change will just roll back.)
*/
- child_att->attinhcount++;
- if (child_att->attinhcount < 0)
+ if (pg_add_s16_overflow(child_att->attinhcount, 1,
+ &child_att->attinhcount))
ereport(ERROR,
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("too many inheritance parents"));
@@ -16075,8 +16076,9 @@ MergeConstraintsIntoExisting(Relation child_rel, Relation parent_rel)
*/
child_copy = heap_copytuple(child_tuple);
child_con = (Form_pg_constraint) GETSTRUCT(child_copy);
- child_con->coninhcount++;
- if (child_con->coninhcount < 0)
+
+ if (pg_add_s16_overflow(child_con->coninhcount, 1,
+ &child_con->coninhcount))
ereport(ERROR,
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
errmsg("too many inheritance parents"));
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index c512824cd1..d6a2c79129 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -41,7 +41,7 @@ typedef struct CookedConstraint
Node *expr; /* transformed default or check expr */
bool skip_validation; /* skip validation? (only for CHECK) */
bool is_local; /* constraint has local (non-inherited) def */
- int inhcount; /* number of times constraint is inherited */
+ int16 inhcount; /* number of times constraint is inherited */
bool is_no_inherit; /* constraint has local def and cannot be
* inherited */
} CookedConstraint;
diff --git a/src/include/nodes/parsenodes.h b/src/include/nodes/parsenodes.h
index 1c314cd907..c37494c135 100644
--- a/src/include/nodes/parsenodes.h
+++ b/src/include/nodes/parsenodes.h
@@ -728,7 +728,7 @@ typedef struct ColumnDef
char *colname; /* name of column */
TypeName *typeName; /* type of column */
char *compression; /* compression method for column */
- int inhcount; /* number of times column is inherited */
+ int16 inhcount; /* number of times column is inherited */
bool is_local; /* column has local (non-inherited) def'n */
bool is_not_null; /* NOT NULL constraint specified? */
bool is_from_type; /* column definition came from table type */
--
2.39.2
* Re: overflow bug for inhcounts
2024-10-08 16:11 overflow bug for inhcounts Alvaro Herrera <alvherre@alvh.no-ip.org>
@ 2024-10-08 16:47 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-10-09 10:59 ` Re: overflow bug for inhcounts Alvaro Herrera <alvherre@alvh.no-ip.org>
2 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2024-10-08 16:47 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>; jian he <jian.universality@gmail.com>
Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> ... This is because ColumnDef->inhcount is a 32-bit int, but
> Form_pg_attribute->attinhcount is int16, so we didn't break the overflow
> test for ColumnDef inhcount, but attinhcount has overflowed during
> assignment.
Ugh ... somebody's ancient oversight there. Or maybe attinhcount
was once int32, and we narrowed it for space reasons?
> From branch master, I propose we change those two members to int16
> (ColumnDef->inhcount and CookedConstraint->inhcount) to make those
> counters consistently use the same type; and then use
> pg_add_s16_overflow() instead of ++ for the increments, as in the
> attached patch. With this patch, the child table creation fails as
> expected ("too many inheritance parents").
+1. I didn't check if there were any other places to touch, but
this looks like a good solution for master.
> In stable branches, I see two possible approaches: we could use the same
> ptach as master (but add another int16 to the struct as padding, to
> avoid changing the struct layout),
That would not preserve ABI on machines with the wrong endianness.
> or, less intrusive, we could leave
> that alone and instead change the "overflow" after the addition to test
> inhcount > PG_INT16_MAX instead of < 0. Or we could leave it all alone.
On the whole I'd leave it alone in back branches. Nobody who's not
intentionally trying to break their table will hit this.
> (I'm not terribly enthused about adding a test that creates a child
> table with 2^16 parents, because of the added runtime -- on my machine
> the scripts above take about 4 seconds.)
Agreed, too expensive for the value.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: overflow bug for inhcounts
2024-10-08 16:11 overflow bug for inhcounts Alvaro Herrera <alvherre@alvh.no-ip.org>
2024-10-08 16:47 ` Re: overflow bug for inhcounts Tom Lane <tgl@sss.pgh.pa.us>
@ 2024-10-09 10:59 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 0 replies; 6+ messages in thread
From: Alvaro Herrera @ 2024-10-09 10:59 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>; jian he <jian.universality@gmail.com>
On 2024-Oct-08, Tom Lane wrote:
> Alvaro Herrera <alvherre@alvh.no-ip.org> writes:
> > ... This is because ColumnDef->inhcount is a 32-bit int, but
> > Form_pg_attribute->attinhcount is int16, so we didn't break the overflow
> > test for ColumnDef inhcount, but attinhcount has overflowed during
> > assignment.
>
> Ugh ... somebody's ancient oversight there. Or maybe attinhcount
> was once int32, and we narrowed it for space reasons?
Yeah, both attinhcount and coninhcount were narrowed in 90189eefc1e1
during REL_16_STABLE development, so it's a relatively new problem.
> > From branch master, I propose we change those two members to int16
> > (ColumnDef->inhcount and CookedConstraint->inhcount) to make those
> > counters consistently use the same type; and then use
> > pg_add_s16_overflow() instead of ++ for the increments, as in the
> > attached patch. With this patch, the child table creation fails as
> > expected ("too many inheritance parents").
>
> +1. I didn't check if there were any other places to touch, but
> this looks like a good solution for master.
Thanks for looking.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
"La conclusión que podemos sacar de esos estudios es que
no podemos sacar ninguna conclusión de ellos" (Tanenbaum)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: overflow bug for inhcounts
2024-10-08 16:11 overflow bug for inhcounts Alvaro Herrera <alvherre@alvh.no-ip.org>
@ 2024-10-08 17:00 ` Andres Freund <andres@anarazel.de>
2 siblings, 0 replies; 6+ messages in thread
From: Andres Freund @ 2024-10-08 17:00 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>; jian he <jian.universality@gmail.com>
Hi,
On 2024-10-08 18:11:39 +0200, Alvaro Herrera wrote:
> Oh and actually, we could change all these variables to be unsigned,
> since there's no use for negative inhcounts. The patch doesn't do that;
> it'd require changing the subtraction paths to use overflow-protected
> ops as well.
Unfortunately we don't really have a way to represent unsigned numbers on the
SQL level today. So I'd not go there for now - it's not like this is a real
limitation for practical use cases.
One case where I'd like unsigned numbers is pg_class.relpages - it's pretty
awkward that it "looks" negative for large tables. 16TB isn't that large
anymore...
Greetings,
Andres
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: overflow bug for inhcounts
2024-10-08 16:11 overflow bug for inhcounts Alvaro Herrera <alvherre@alvh.no-ip.org>
@ 2024-10-09 02:01 ` jian he <jian.universality@gmail.com>
2024-10-10 16:38 ` Re: overflow bug for inhcounts Alvaro Herrera <alvherre@alvh.no-ip.org>
2 siblings, 1 reply; 6+ messages in thread
From: jian he @ 2024-10-09 02:01 UTC (permalink / raw)
To: Alvaro Herrera <alvherre@alvh.no-ip.org>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
Oid
CreateConstraintEntry(const char *constraintName,
Oid constraintNamespace,
char constraintType,
....
int conInhCount,
bool conNoInherit,
bool conPeriod,
bool is_internal)
we can change
int conInhCount,
to
int16 conInhCount.
doing that, meaning we also need to refactor some of the caller functions.
we can also change
node Constraint field inhcount from int to int16.
the attached patch doing it.
while at it, minor changed CreateConstraintEntry caller's comments.
based on your patch.
Attachments:
further_refactor_intcount.no-cfbot (application/octet-stream, 3.9K) download
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: overflow bug for inhcounts
2024-10-08 16:11 overflow bug for inhcounts Alvaro Herrera <alvherre@alvh.no-ip.org>
2024-10-09 02:01 ` Re: overflow bug for inhcounts jian he <jian.universality@gmail.com>
@ 2024-10-10 16:38 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
0 siblings, 0 replies; 6+ messages in thread
From: Alvaro Herrera @ 2024-10-10 16:38 UTC (permalink / raw)
To: jian he <jian.universality@gmail.com>; +Cc: Pg Hackers <pgsql-hackers@lists.postgresql.org>
On 2024-Oct-09, jian he wrote:
> CreateConstraintEntry(const char *constraintName,
> we can change
> int conInhCount,
> to
> int16 conInhCount.
> doing that, meaning we also need to refactor some of the caller functions.
Good thought, thanks.
> we can also change
> node Constraint field inhcount from int to int16.
Nah, that field is unused, so I removed it. I gave another look at the
not-nulls branch, and we can do without it.
This is pushed, although there was a technical glitch (since fixed by
Magnus Hagander) and the pgsql-committers message was not sent. But you
can see it here:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=fd64ed60b62697984bb69a09a3ae19fbe...
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2024-10-10 16:38 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-10-08 16:11 overflow bug for inhcounts Alvaro Herrera <alvherre@alvh.no-ip.org>
2024-10-08 16:47 ` Tom Lane <tgl@sss.pgh.pa.us>
2024-10-09 10:59 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
2024-10-08 17:00 ` Andres Freund <andres@anarazel.de>
2024-10-09 02:01 ` jian he <jian.universality@gmail.com>
2024-10-10 16:38 ` Alvaro Herrera <alvherre@alvh.no-ip.org>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox