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)
>From 45ff2614e85c47b339e04da147dff74995a0540c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?=C3=81lvaro=20Herrera?= <alvhe...@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
