On 21.03.23 18:46, Andres Freund wrote:
I don't think we can find enough to make the impact zero bytes. It's also
not clear exactly what the impact of each byte would be (compared to
possible complications in other parts of the code, for example). But if
there are a few low-hanging fruit, it seems like we could pick them, to old
us over until we have a better solution to the underlying issue.
attndims 4->2
attstattarget 4->2
attinhcount 4->2
+ some reordering only gets you from 112->108 unfortunately, due to a 1 byte
alignment hole and 2 bytes of trailing padding.
before:
/* size: 112, cachelines: 2, members: 22 */
/* sum members: 111, holes: 1, sum holes: 1 */
/* last cacheline: 48 bytes */
after:
/* size: 108, cachelines: 2, members: 22 */
/* sum members: 105, holes: 1, sum holes: 1 */
/* padding: 2 */
/* last cacheline: 44 bytes */
You might be able to fill the hole + padding with your data - but IIRC that
was 3 4byte integers?
Here is an updated patch that handles those three fields, including some
overflow checks. I also changed coninhcount to match attinhcount.
I structured the inhcount overflow checks to be independent of the
integer size, but maybe others find this approach weird.
Given the calculation shown, there is no value in reducing all three
fields versus just two, but I don't find compelling reasons to leave out
one or the other field. (attstattarget got the most discussion, but
that one is actually the easiest part of the patch.)
I took another hard look at some of the other proposals, including
moving some fields to the variable length part or combining some bool or
char fields. Those changes all appear to have a really long tail of
issues all over the code that I wouldn't want to attack them now in an
ad hoc way.
My suggestion is to use this patch and then consider the column
encryption patch as it stands now.
The discussion about attcacheoff seems to be still ongoing. But it
seems whatever the outcome would be independent of this patch: Either we
keep it or we remove it; there is no proposal to resize it.
From cff1daee2f12b4d625b18e6a89cb5e033f7fdfa5 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Thu, 23 Mar 2023 11:46:05 +0100
Subject: [PATCH v2] Save a few bytes in pg_attribute
attndims, attstattarget, and attinhcount can be converted to int16
(from int32).
---
doc/src/sgml/catalogs.sgml | 60 ++++++++++++++---------------
src/backend/access/common/tupdesc.c | 8 ++++
src/backend/catalog/heap.c | 11 ++++--
src/backend/catalog/index.c | 2 +-
src/backend/catalog/pg_constraint.c | 6 ++-
src/backend/commands/tablecmds.c | 28 ++++++++++++++
src/include/catalog/pg_attribute.h | 34 ++++++++--------
src/include/catalog/pg_constraint.h | 2 +-
8 files changed, 99 insertions(+), 52 deletions(-)
diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 746baf5053..7c09ab3000 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1164,23 +1164,6 @@ <title><structname>pg_attribute</structname>
Columns</title>
</para></entry>
</row>
- <row>
- <entry role="catalog_table_entry"><para role="column_definition">
- <structfield>attstattarget</structfield> <type>int4</type>
- </para>
- <para>
- <structfield>attstattarget</structfield> controls the level of detail
- of statistics accumulated for this column by
- <link linkend="sql-analyze"><command>ANALYZE</command></link>.
- A zero value indicates that no statistics should be collected.
- A negative value says to use the system default statistics target.
- The exact meaning of positive values is data type-dependent.
- For scalar data types, <structfield>attstattarget</structfield>
- is both the target number of <quote>most common values</quote>
- to collect, and the target number of histogram bins to create.
- </para></entry>
- </row>
-
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>attlen</structfield> <type>int2</type>
@@ -1202,17 +1185,6 @@ <title><structname>pg_attribute</structname>
Columns</title>
</para></entry>
</row>
- <row>
- <entry role="catalog_table_entry"><para role="column_definition">
- <structfield>attndims</structfield> <type>int4</type>
- </para>
- <para>
- Number of dimensions, if the column is an array type; otherwise 0.
- (Presently, the number of dimensions of an array is not enforced,
- so any nonzero value effectively means <quote>it's an array</quote>.)
- </para></entry>
- </row>
-
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>attcacheoff</structfield> <type>int4</type>
@@ -1237,6 +1209,17 @@ <title><structname>pg_attribute</structname>
Columns</title>
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>attndims</structfield> <type>int2</type>
+ </para>
+ <para>
+ Number of dimensions, if the column is an array type; otherwise 0.
+ (Presently, the number of dimensions of an array is not enforced,
+ so any nonzero value effectively means <quote>it's an array</quote>.)
+ </para></entry>
+ </row>
+
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>attbyval</structfield> <type>bool</type>
@@ -1362,7 +1345,7 @@ <title><structname>pg_attribute</structname>
Columns</title>
<row>
<entry role="catalog_table_entry"><para role="column_definition">
- <structfield>attinhcount</structfield> <type>int4</type>
+ <structfield>attinhcount</structfield> <type>int2</type>
</para>
<para>
The number of direct ancestors this column has. A column with a
@@ -1370,6 +1353,23 @@ <title><structname>pg_attribute</structname>
Columns</title>
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>attstattarget</structfield> <type>int2</type>
+ </para>
+ <para>
+ <structfield>attstattarget</structfield> controls the level of detail
+ of statistics accumulated for this column by
+ <link linkend="sql-analyze"><command>ANALYZE</command></link>.
+ A zero value indicates that no statistics should be collected.
+ A negative value says to use the system default statistics target.
+ The exact meaning of positive values is data type-dependent.
+ For scalar data types, <structfield>attstattarget</structfield>
+ is both the target number of <quote>most common values</quote>
+ to collect, and the target number of histogram bins to create.
+ </para></entry>
+ </row>
+
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>attcollation</structfield> <type>oid</type>
@@ -2691,7 +2691,7 @@ <title><structname>pg_constraint</structname>
Columns</title>
<row>
<entry role="catalog_table_entry"><para role="column_definition">
- <structfield>coninhcount</structfield> <type>int4</type>
+ <structfield>coninhcount</structfield> <type>int2</type>
</para>
<para>
The number of direct inheritance ancestors this constraint has.
diff --git a/src/backend/access/common/tupdesc.c
b/src/backend/access/common/tupdesc.c
index 72a2c3d3db..7c5c390503 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -597,6 +597,8 @@ TupleDescInitEntry(TupleDesc desc,
Assert(PointerIsValid(desc));
Assert(attributeNumber >= 1);
Assert(attributeNumber <= desc->natts);
+ Assert(attdim >= 0);
+ Assert(attdim <= PG_INT16_MAX);
/*
* initialize the attribute fields
@@ -667,6 +669,8 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
Assert(PointerIsValid(desc));
Assert(attributeNumber >= 1);
Assert(attributeNumber <= desc->natts);
+ Assert(attdim >= 0);
+ Assert(attdim <= PG_INT16_MAX);
/* initialize the attribute fields */
att = TupleDescAttr(desc, attributeNumber - 1);
@@ -827,6 +831,10 @@ BuildDescForRelation(List *schema)
attcollation = GetColumnDefCollation(NULL, entry, atttypid);
attdim = list_length(entry->typeName->arrayBounds);
+ if (attdim > PG_INT16_MAX)
+ ereport(ERROR,
+ errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("too many array dimensions"));
if (entry->typeName->setof)
ereport(ERROR,
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 4f006820b8..2a0d82aedd 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -732,12 +732,11 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
slot[slotCount]->tts_values[Anum_pg_attribute_attname - 1] =
NameGetDatum(&attrs->attname);
slot[slotCount]->tts_values[Anum_pg_attribute_atttypid - 1] =
ObjectIdGetDatum(attrs->atttypid);
- slot[slotCount]->tts_values[Anum_pg_attribute_attstattarget -
1] = Int32GetDatum(attrs->attstattarget);
slot[slotCount]->tts_values[Anum_pg_attribute_attlen - 1] =
Int16GetDatum(attrs->attlen);
slot[slotCount]->tts_values[Anum_pg_attribute_attnum - 1] =
Int16GetDatum(attrs->attnum);
- slot[slotCount]->tts_values[Anum_pg_attribute_attndims - 1] =
Int32GetDatum(attrs->attndims);
slot[slotCount]->tts_values[Anum_pg_attribute_attcacheoff - 1]
= Int32GetDatum(-1);
slot[slotCount]->tts_values[Anum_pg_attribute_atttypmod - 1] =
Int32GetDatum(attrs->atttypmod);
+ slot[slotCount]->tts_values[Anum_pg_attribute_attndims - 1] =
Int16GetDatum(attrs->attndims);
slot[slotCount]->tts_values[Anum_pg_attribute_attbyval - 1] =
BoolGetDatum(attrs->attbyval);
slot[slotCount]->tts_values[Anum_pg_attribute_attalign - 1] =
CharGetDatum(attrs->attalign);
slot[slotCount]->tts_values[Anum_pg_attribute_attstorage - 1] =
CharGetDatum(attrs->attstorage);
@@ -749,7 +748,8 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
slot[slotCount]->tts_values[Anum_pg_attribute_attgenerated - 1]
= CharGetDatum(attrs->attgenerated);
slot[slotCount]->tts_values[Anum_pg_attribute_attisdropped - 1]
= BoolGetDatum(attrs->attisdropped);
slot[slotCount]->tts_values[Anum_pg_attribute_attislocal - 1] =
BoolGetDatum(attrs->attislocal);
- slot[slotCount]->tts_values[Anum_pg_attribute_attinhcount - 1]
= Int32GetDatum(attrs->attinhcount);
+ slot[slotCount]->tts_values[Anum_pg_attribute_attinhcount - 1]
= Int16GetDatum(attrs->attinhcount);
+ slot[slotCount]->tts_values[Anum_pg_attribute_attstattarget -
1] = Int16GetDatum(attrs->attstattarget);
slot[slotCount]->tts_values[Anum_pg_attribute_attcollation - 1]
= ObjectIdGetDatum(attrs->attcollation);
if (attoptions && attoptions[natts] != (Datum) 0)
slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] =
attoptions[natts];
@@ -2615,6 +2615,11 @@ MergeWithExistingConstraint(Relation rel, const char
*ccname, Node *expr,
con->conislocal = true;
else
con->coninhcount++;
+
+ if (con->coninhcount < 0)
+ ereport(ERROR,
+
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("too many inheritance
parents"));
}
if (is_no_inherit)
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 33e3d0ec05..4992ca1fce 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -1813,7 +1813,7 @@ index_concurrently_swap(Oid newIndexId, Oid oldIndexId,
const char *oldName)
memset(repl_repl, false, sizeof(repl_repl));
repl_repl[Anum_pg_attribute_attstattarget - 1] = true;
- repl_val[Anum_pg_attribute_attstattarget - 1] =
Int32GetDatum(attstattarget);
+ repl_val[Anum_pg_attribute_attstattarget - 1] =
Int16GetDatum(attstattarget);
newTuple = heap_modify_tuple(attrTuple,
RelationGetDescr(pg_attribute),
diff --git a/src/backend/catalog/pg_constraint.c
b/src/backend/catalog/pg_constraint.c
index 7392c72e90..fb684edfa9 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -190,7 +190,7 @@ CreateConstraintEntry(const char *constraintName,
values[Anum_pg_constraint_confdeltype - 1] =
CharGetDatum(foreignDeleteType);
values[Anum_pg_constraint_confmatchtype - 1] =
CharGetDatum(foreignMatchType);
values[Anum_pg_constraint_conislocal - 1] = BoolGetDatum(conIsLocal);
- values[Anum_pg_constraint_coninhcount - 1] = Int32GetDatum(conInhCount);
+ values[Anum_pg_constraint_coninhcount - 1] = Int16GetDatum(conInhCount);
values[Anum_pg_constraint_connoinherit - 1] =
BoolGetDatum(conNoInherit);
if (conkeyArray)
@@ -805,6 +805,10 @@ ConstraintSetParentConstraint(Oid childConstrId,
constrForm->conislocal = false;
constrForm->coninhcount++;
+ if (constrForm->coninhcount < 0)
+ 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 3e2c5f797c..27860e313e 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -2649,6 +2649,10 @@ MergeAttributes(List *schema, List *supers, char
relpersistence,
*/
def->inhcount++;
+ if (def->inhcount < 0)
+ ereport(ERROR,
+
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("too many
inheritance parents"));
newattmap->attnums[parent_attno - 1] =
exist_attno;
}
@@ -3172,6 +3176,10 @@ MergeCheckConstraint(List *constraints, char *name, Node
*expr)
{
/* OK to merge */
ccon->inhcount++;
+ if (ccon->inhcount < 0)
+ ereport(ERROR,
+
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("too many inheritance
parents"));
return true;
}
@@ -6787,6 +6795,10 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab,
Relation rel,
/* Bump the existing child att's inhcount */
childatt->attinhcount++;
+ if (childatt->attinhcount < 0)
+ ereport(ERROR,
+
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("too many inheritance
parents"));
CatalogTupleUpdate(attrdesc, &tuple->t_self, tuple);
heap_freetuple(tuple);
@@ -6878,6 +6890,10 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab,
Relation rel,
attribute.attstattarget = (newattnum > 0) ? -1 : 0;
attribute.attlen = tform->typlen;
attribute.attnum = newattnum;
+ if (list_length(colDef->typeName->arrayBounds) > PG_INT16_MAX)
+ ereport(ERROR,
+ errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("too many array dimensions"));
attribute.attndims = list_length(colDef->typeName->arrayBounds);
attribute.atttypmod = typmod;
attribute.attbyval = tform->typbyval;
@@ -12890,6 +12906,10 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation
rel,
attTup->atttypid = targettype;
attTup->atttypmod = targettypmod;
attTup->attcollation = targetcollid;
+ if (list_length(typeName->arrayBounds) > PG_INT16_MAX)
+ ereport(ERROR,
+ errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("too many array dimensions"));
attTup->attndims = list_length(typeName->arrayBounds);
attTup->attlen = tform->typlen;
attTup->attbyval = tform->typbyval;
@@ -15124,6 +15144,10 @@ MergeAttributesIntoExisting(Relation child_rel,
Relation parent_rel)
* later on, this change will just roll back.)
*/
childatt->attinhcount++;
+ if (childatt->attinhcount < 0)
+ ereport(ERROR,
+
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("too many inheritance
parents"));
/*
* In case of partitions, we must enforce that value of
attislocal
@@ -15261,6 +15285,10 @@ 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)
+ ereport(ERROR,
+
errcode(ERRCODE_PROGRAM_LIMIT_EXCEEDED),
+ errmsg("too many inheritance
parents"));
/*
* In case of partitions, an inherited constraint must
be
diff --git a/src/include/catalog/pg_attribute.h
b/src/include/catalog/pg_attribute.h
index b561e17781..f8b4861b94 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -52,15 +52,6 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP
BKI_ROWTYPE_OID(75,
*/
Oid atttypid BKI_LOOKUP_OPT(pg_type);
- /*
- * attstattarget is the target number of statistics datapoints to
collect
- * during VACUUM ANALYZE of this column. A zero here indicates that we
do
- * not wish to collect any stats about this column. A "-1" here
indicates
- * that no value has been explicitly set for this column, so ANALYZE
- * should use the default setting.
- */
- int32 attstattarget BKI_DEFAULT(-1);
-
/*
* attlen is a copy of the typlen field from pg_type for this attribute.
* See atttypid comments above.
@@ -82,12 +73,6 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP
BKI_ROWTYPE_OID(75,
*/
int16 attnum;
- /*
- * attndims is the declared number of dimensions, if an array type,
- * otherwise zero.
- */
- int32 attndims;
-
/*
* fastgetattr() uses attcacheoff to cache byte offsets of attributes in
* heap tuples. The value actually stored in pg_attribute (-1)
indicates
@@ -105,6 +90,12 @@ CATALOG(pg_attribute,1249,AttributeRelationId)
BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
*/
int32 atttypmod BKI_DEFAULT(-1);
+ /*
+ * attndims is the declared number of dimensions, if an array type,
+ * otherwise zero.
+ */
+ int16 attndims;
+
/*
* attbyval is a copy of the typbyval field from pg_type for this
* attribute. See atttypid comments above.
@@ -165,7 +156,18 @@ CATALOG(pg_attribute,1249,AttributeRelationId)
BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
bool attislocal BKI_DEFAULT(t);
/* Number of times inherited from direct parent relation(s) */
- int32 attinhcount BKI_DEFAULT(0);
+ int16 attinhcount BKI_DEFAULT(0);
+
+ /*
+ * attstattarget is the target number of statistics datapoints to
collect
+ * during VACUUM ANALYZE of this column. A zero here indicates that we
do
+ * not wish to collect any stats about this column. A "-1" here
indicates
+ * that no value has been explicitly set for this column, so ANALYZE
+ * should use the default setting.
+ *
+ * int16 is sufficient because the max value is currently 10000.
+ */
+ int16 attstattarget BKI_DEFAULT(-1);
/* attribute's collation, if any */
Oid attcollation BKI_LOOKUP_OPT(pg_collation);
diff --git a/src/include/catalog/pg_constraint.h
b/src/include/catalog/pg_constraint.h
index 96889fddfa..16bf5f5576 100644
--- a/src/include/catalog/pg_constraint.h
+++ b/src/include/catalog/pg_constraint.h
@@ -102,7 +102,7 @@ CATALOG(pg_constraint,2606,ConstraintRelationId)
bool conislocal;
/* Number of times inherited from direct parent relation(s) */
- int32 coninhcount;
+ int16 coninhcount;
/* Has a local definition and cannot be inherited */
bool connoinherit;
base-commit: ecb696527c01908d54b7a7aa2bd9179585b46459
--
2.40.0
