Re: Move pg_attribute.attcompression to earlier in struct for reduced size?

Michael Paquier <michael@paquier.xyz> writes:
> This is still an open item.  FWIW, I can get behind the reordering
> proposed by Tom for the consistency gained with pg_type, leading to
> the attached to reduce the size of FormData_pg_attribute from 116b to
> 112b.

I think we need to do more than that.  It's certainly not okay to
leave catalogs.sgml out of sync with reality.  And maybe I'm just
an overly anal-retentive sort, but I think that code that manipulates
tuples ought to match the declared field order if there's not some
specific reason to do otherwise.  So that led me to the attached.

It was a good thing I went through this code, too, because I noticed
one serious bug (attcompression not checked in equalTupleDescs) and
another thing that looks like a bug: there are two places that set
up attcompression depending on

    if (rel->rd_rel->relkind == RELKIND_RELATION ||
        rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)

This seems fairly nuts; in particular, why are matviews excluded?

            regards, tom lane

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 6d06ad22b9..8aebc4d12f 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -1236,6 +1236,15 @@
       </para></entry>
      </row>

+     <row>
+      <entry role="catalog_table_entry"><para role="column_definition">
+       <structfield>attalign</structfield> <type>char</type>
+      </para>
+      <para>
+       A copy of <literal>pg_type.typalign</literal> of this column's type
+      </para></entry>
+     </row>
+
      <row>
       <entry role="catalog_table_entry"><para role="column_definition">
        <structfield>attstorage</structfield> <type>char</type>
@@ -1249,10 +1258,13 @@

      <row>
       <entry role="catalog_table_entry"><para role="column_definition">
-       <structfield>attalign</structfield> <type>char</type>
+       <structfield>attcompression</structfield> <type>char</type>
       </para>
       <para>
-       A copy of <literal>pg_type.typalign</literal> of this column's type
+       The current compression method of the column.  If it is an invalid
+       compression method (<literal>'\0'</literal>) then column data will not
+       be compressed.  Otherwise, <literal>'p'</literal> = pglz compression or
+       <literal>'l'</literal> = <productname>LZ4</productname> compression.
       </para></entry>
      </row>

@@ -1355,18 +1367,6 @@
       </para></entry>
      </row>

-     <row>
-      <entry role="catalog_table_entry"><para role="column_definition">
-       <structfield>attcompression</structfield> <type>char</type>
-      </para>
-      <para>
-       The current compression method of the column.  If it is an invalid
-       compression method (<literal>'\0'</literal>) then column data will not
-       be compressed.  Otherwise, <literal>'p'</literal> = pglz compression or
-       <literal>'l'</literal> = <productname>LZ4</productname> compression.
-      </para></entry>
-     </row>
-
      <row>
       <entry role="catalog_table_entry"><para role="column_definition">
        <structfield>attacl</structfield> <type>aclitem[]</type>
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index ffb275afbe..e03e5c9909 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -440,9 +440,11 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
             return false;
         if (attr1->attbyval != attr2->attbyval)
             return false;
+        if (attr1->attalign != attr2->attalign)
+            return false;
         if (attr1->attstorage != attr2->attstorage)
             return false;
-        if (attr1->attalign != attr2->attalign)
+        if (attr1->attcompression != attr2->attcompression)
             return false;
         if (attr1->attnotnull != attr2->attnotnull)
             return false;
@@ -460,7 +462,7 @@ equalTupleDescs(TupleDesc tupdesc1, TupleDesc tupdesc2)
             return false;
         if (attr1->attcollation != attr2->attcollation)
             return false;
-        /* attacl, attoptions and attfdwoptions are not even present... */
+        /* variable-length fields are not even present... */
     }

     if (tupdesc1->constr != NULL)
@@ -639,12 +641,11 @@ TupleDescInitEntry(TupleDesc desc,
     att->attbyval = typeForm->typbyval;
     att->attalign = typeForm->typalign;
     att->attstorage = typeForm->typstorage;
-    att->attcollation = typeForm->typcollation;
-
     if (IsStorageCompressible(typeForm->typstorage))
         att->attcompression = GetDefaultToastCompression();
     else
         att->attcompression = InvalidCompressionMethod;
+    att->attcollation = typeForm->typcollation;

     ReleaseSysCache(tuple);
 }
@@ -709,6 +710,7 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
             att->attbyval = false;
             att->attalign = TYPALIGN_INT;
             att->attstorage = TYPSTORAGE_EXTENDED;
+            att->attcompression = InvalidCompressionMethod; /* don't care */
             att->attcollation = DEFAULT_COLLATION_OID;
             break;

@@ -717,6 +719,7 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
             att->attbyval = true;
             att->attalign = TYPALIGN_CHAR;
             att->attstorage = TYPSTORAGE_PLAIN;
+            att->attcompression = InvalidCompressionMethod;
             att->attcollation = InvalidOid;
             break;

@@ -725,6 +728,7 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
             att->attbyval = true;
             att->attalign = TYPALIGN_INT;
             att->attstorage = TYPSTORAGE_PLAIN;
+            att->attcompression = InvalidCompressionMethod;
             att->attcollation = InvalidOid;
             break;

@@ -733,6 +737,7 @@ TupleDescInitBuiltinEntry(TupleDesc desc,
             att->attbyval = FLOAT8PASSBYVAL;
             att->attalign = TYPALIGN_DOUBLE;
             att->attstorage = TYPSTORAGE_PLAIN;
+            att->attcompression = InvalidCompressionMethod;
             att->attcollation = InvalidOid;
             break;

diff --git a/src/backend/access/spgist/spgutils.c b/src/backend/access/spgist/spgutils.c
index 8d99c9b762..9ff280a252 100644
--- a/src/backend/access/spgist/spgutils.c
+++ b/src/backend/access/spgist/spgutils.c
@@ -165,8 +165,8 @@ fillTypeDesc(SpGistTypeDesc *desc, Oid type)
     typtup = (Form_pg_type) GETSTRUCT(tp);
     desc->attlen = typtup->typlen;
     desc->attbyval = typtup->typbyval;
-    desc->attstorage = typtup->typstorage;
     desc->attalign = typtup->typalign;
+    desc->attstorage = typtup->typstorage;
     ReleaseSysCache(tp);
 }

@@ -304,8 +304,8 @@ getSpGistTupleDesc(Relation index, SpGistTypeDesc *keyType)
         att->attalign = keyType->attalign;
         att->attstorage = keyType->attstorage;
         /* We shouldn't need to bother with making these valid: */
-        att->attcollation = InvalidOid;
         att->attcompression = InvalidCompressionMethod;
+        att->attcollation = InvalidOid;
         /* In case we changed typlen, we'd better reset following offsets */
         for (int i = spgFirstIncludeColumn; i < outTupDesc->natts; i++)
             TupleDescAttr(outTupDesc, i)->attcacheoff = -1;
diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
index b155237488..217b4cd001 100644
--- a/src/backend/bootstrap/bootstrap.c
+++ b/src/backend/bootstrap/bootstrap.c
@@ -699,8 +699,8 @@ DefineAttr(char *name, char *type, int attnum, int nullness)
         attrtypes[attnum]->atttypid = Ap->am_oid;
         attrtypes[attnum]->attlen = Ap->am_typ.typlen;
         attrtypes[attnum]->attbyval = Ap->am_typ.typbyval;
-        attrtypes[attnum]->attstorage = Ap->am_typ.typstorage;
         attrtypes[attnum]->attalign = Ap->am_typ.typalign;
+        attrtypes[attnum]->attstorage = Ap->am_typ.typstorage;
         attrtypes[attnum]->attcollation = Ap->am_typ.typcollation;
         /* if an array type, assume 1-dimensional attribute */
         if (Ap->am_typ.typelem != InvalidOid && Ap->am_typ.typlen < 0)
@@ -713,8 +713,8 @@ DefineAttr(char *name, char *type, int attnum, int nullness)
         attrtypes[attnum]->atttypid = TypInfo[typeoid].oid;
         attrtypes[attnum]->attlen = TypInfo[typeoid].len;
         attrtypes[attnum]->attbyval = TypInfo[typeoid].byval;
-        attrtypes[attnum]->attstorage = TypInfo[typeoid].storage;
         attrtypes[attnum]->attalign = TypInfo[typeoid].align;
+        attrtypes[attnum]->attstorage = TypInfo[typeoid].storage;
         attrtypes[attnum]->attcollation = TypInfo[typeoid].collation;
         /* if an array type, assume 1-dimensional attribute */
         if (TypInfo[typeoid].elem != InvalidOid &&
diff --git a/src/backend/catalog/genbki.pl b/src/backend/catalog/genbki.pl
index 63a907d50d..112b3affdf 100644
--- a/src/backend/catalog/genbki.pl
+++ b/src/backend/catalog/genbki.pl
@@ -897,8 +897,11 @@ sub morph_row_for_pgattr
     $row->{atttypid}   = $type->{oid};
     $row->{attlen}     = $type->{typlen};
     $row->{attbyval}   = $type->{typbyval};
-    $row->{attstorage} = $type->{typstorage};
     $row->{attalign}   = $type->{typalign};
+    $row->{attstorage} = $type->{typstorage};
+
+    $row->{attcompression} =
+      $type->{typstorage} ne 'p' && $type->{typstorage} ne 'e' ? 'p' : '\0';

     # set attndims if it's an array type
     $row->{attndims} = $type->{typcategory} eq 'A' ? '1' : '0';
@@ -907,9 +910,6 @@ sub morph_row_for_pgattr
     $row->{attcollation} =
       $type->{typcollation} ne '0' ? $C_COLLATION_OID : 0;

-    $row->{attcompression} =
-      $type->{typstorage} ne 'p' && $type->{typstorage} ne 'e' ? 'p' : '\0';
-
     if (defined $attr->{forcenotnull})
     {
         $row->{attnotnull} = 't';
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 431e62e389..3b2cd203c1 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -146,7 +146,8 @@ static Node *cookConstraint(ParseState *pstate,
 /*
  * The initializers below do not include trailing variable length fields,
  * but that's OK - we're never going to reference anything beyond the
- * fixed-size portion of the structure anyway.
+ * fixed-size portion of the structure anyway.  Fields that can default
+ * to zeroes are also not mentioned.
  */

 static const FormData_pg_attribute a1 = {
@@ -157,8 +158,8 @@ static const FormData_pg_attribute a1 = {
     .attcacheoff = -1,
     .atttypmod = -1,
     .attbyval = false,
-    .attstorage = TYPSTORAGE_PLAIN,
     .attalign = TYPALIGN_SHORT,
+    .attstorage = TYPSTORAGE_PLAIN,
     .attnotnull = true,
     .attislocal = true,
 };
@@ -171,8 +172,8 @@ static const FormData_pg_attribute a2 = {
     .attcacheoff = -1,
     .atttypmod = -1,
     .attbyval = true,
-    .attstorage = TYPSTORAGE_PLAIN,
     .attalign = TYPALIGN_INT,
+    .attstorage = TYPSTORAGE_PLAIN,
     .attnotnull = true,
     .attislocal = true,
 };
@@ -185,8 +186,8 @@ static const FormData_pg_attribute a3 = {
     .attcacheoff = -1,
     .atttypmod = -1,
     .attbyval = true,
-    .attstorage = TYPSTORAGE_PLAIN,
     .attalign = TYPALIGN_INT,
+    .attstorage = TYPSTORAGE_PLAIN,
     .attnotnull = true,
     .attislocal = true,
 };
@@ -199,8 +200,8 @@ static const FormData_pg_attribute a4 = {
     .attcacheoff = -1,
     .atttypmod = -1,
     .attbyval = true,
-    .attstorage = TYPSTORAGE_PLAIN,
     .attalign = TYPALIGN_INT,
+    .attstorage = TYPSTORAGE_PLAIN,
     .attnotnull = true,
     .attislocal = true,
 };
@@ -213,8 +214,8 @@ static const FormData_pg_attribute a5 = {
     .attcacheoff = -1,
     .atttypmod = -1,
     .attbyval = true,
-    .attstorage = TYPSTORAGE_PLAIN,
     .attalign = TYPALIGN_INT,
+    .attstorage = TYPSTORAGE_PLAIN,
     .attnotnull = true,
     .attislocal = true,
 };
@@ -233,8 +234,8 @@ static const FormData_pg_attribute a6 = {
     .attcacheoff = -1,
     .atttypmod = -1,
     .attbyval = true,
-    .attstorage = TYPSTORAGE_PLAIN,
     .attalign = TYPALIGN_INT,
+    .attstorage = TYPSTORAGE_PLAIN,
     .attnotnull = true,
     .attislocal = true,
 };
@@ -779,8 +780,9 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
         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_attbyval - 1] = BoolGetDatum(attrs->attbyval);
-        slot[slotCount]->tts_values[Anum_pg_attribute_attstorage - 1] = CharGetDatum(attrs->attstorage);
         slot[slotCount]->tts_values[Anum_pg_attribute_attalign - 1] = CharGetDatum(attrs->attalign);
+        slot[slotCount]->tts_values[Anum_pg_attribute_attstorage - 1] = CharGetDatum(attrs->attstorage);
+        slot[slotCount]->tts_values[Anum_pg_attribute_attcompression - 1] = CharGetDatum(attrs->attcompression);
         slot[slotCount]->tts_values[Anum_pg_attribute_attnotnull - 1] = BoolGetDatum(attrs->attnotnull);
         slot[slotCount]->tts_values[Anum_pg_attribute_atthasdef - 1] = BoolGetDatum(attrs->atthasdef);
         slot[slotCount]->tts_values[Anum_pg_attribute_atthasmissing - 1] = BoolGetDatum(attrs->atthasmissing);
@@ -790,7 +792,6 @@ InsertPgAttributeTuples(Relation pg_attribute_rel,
         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_attcollation - 1] = ObjectIdGetDatum(attrs->attcollation);
-        slot[slotCount]->tts_values[Anum_pg_attribute_attcompression - 1] = CharGetDatum(attrs->attcompression);
         if (attoptions && attoptions[natts] != (Datum) 0)
             slot[slotCount]->tts_values[Anum_pg_attribute_attoptions - 1] = attoptions[natts];
         else
diff --git a/src/backend/catalog/index.c b/src/backend/catalog/index.c
index 0f8cfae4ec..30b900aac6 100644
--- a/src/backend/catalog/index.c
+++ b/src/backend/catalog/index.c
@@ -345,8 +345,8 @@ ConstructTupleDescriptor(Relation heapRelation,
             to->attndims = from->attndims;
             to->atttypmod = from->atttypmod;
             to->attbyval = from->attbyval;
-            to->attstorage = from->attstorage;
             to->attalign = from->attalign;
+            to->attstorage = from->attstorage;
             to->attcompression = from->attcompression;
         }
         else
@@ -373,10 +373,10 @@ ConstructTupleDescriptor(Relation heapRelation,
              */
             to->atttypid = keyType;
             to->attlen = typeTup->typlen;
+            to->atttypmod = exprTypmod(indexkey);
             to->attbyval = typeTup->typbyval;
-            to->attstorage = typeTup->typstorage;
             to->attalign = typeTup->typalign;
-            to->atttypmod = exprTypmod(indexkey);
+            to->attstorage = typeTup->typstorage;

             /*
              * For expression columns, set attcompression invalid, since
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ebc62034d2..e28d6e4253 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -6593,12 +6593,19 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
     attribute.atttypid = typeOid;
     attribute.attstattarget = (newattnum > 0) ? -1 : 0;
     attribute.attlen = tform->typlen;
-    attribute.atttypmod = typmod;
     attribute.attnum = newattnum;
-    attribute.attbyval = tform->typbyval;
     attribute.attndims = list_length(colDef->typeName->arrayBounds);
-    attribute.attstorage = tform->typstorage;
+    attribute.atttypmod = typmod;
+    attribute.attbyval = tform->typbyval;
     attribute.attalign = tform->typalign;
+    attribute.attstorage = tform->typstorage;
+    /* do not set compression in views etc */
+    if (rel->rd_rel->relkind == RELKIND_RELATION ||
+        rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
+        attribute.attcompression = GetAttributeCompression(&attribute,
+                                                           colDef->compression);
+    else
+        attribute.attcompression = InvalidCompressionMethod;
     attribute.attnotnull = colDef->is_not_null;
     attribute.atthasdef = false;
     attribute.atthasmissing = false;
@@ -6609,17 +6616,6 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
     attribute.attinhcount = colDef->inhcount;
     attribute.attcollation = collOid;

-    /*
-     * lookup attribute's compression method and store it in the
-     * attr->attcompression.
-     */
-    if (rel->rd_rel->relkind == RELKIND_RELATION ||
-        rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE)
-        attribute.attcompression = GetAttributeCompression(&attribute,
-                                                           colDef->compression);
-    else
-        attribute.attcompression = InvalidCompressionMethod;
-
     /* attribute.attacl is handled by InsertPgAttributeTuples() */

     ReleaseSysCache(typeTuple);
diff --git a/src/include/access/spgist_private.h b/src/include/access/spgist_private.h
index ba3da5b540..40d3b71b89 100644
--- a/src/include/access/spgist_private.h
+++ b/src/include/access/spgist_private.h
@@ -137,8 +137,8 @@ typedef struct SpGistTypeDesc
     Oid            type;
     int16        attlen;
     bool        attbyval;
-    char        attstorage;
     char        attalign;
+    char        attstorage;
 } SpGistTypeDesc;

 typedef struct SpGistState
diff --git a/src/include/catalog/pg_attribute.h b/src/include/catalog/pg_attribute.h
index 560f8f00bb..1e26016214 100644
--- a/src/include/catalog/pg_attribute.h
+++ b/src/include/catalog/pg_attribute.h
@@ -111,6 +111,12 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
      */
     bool        attbyval;

+    /*
+     * attalign is a copy of the typalign field from pg_type for this
+     * attribute.  See atttypid comments above.
+     */
+    char        attalign;
+
     /*----------
      * attstorage tells for VARLENA attributes, what the heap access
      * methods can do to it if a given tuple doesn't fit into a page.
@@ -120,10 +126,10 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
     char        attstorage;

     /*
-     * attalign is a copy of the typalign field from pg_type for this
-     * attribute.  See atttypid comments above.
+     * Compression method.  Must be InvalidCompressionMethod if and only if
+     * typstorage is 'plain' or 'external'.
      */
-    char        attalign;
+    char        attcompression BKI_DEFAULT('\0');

     /* This flag represents the "NOT NULL" constraint */
     bool        attnotnull;
@@ -160,12 +166,6 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
     /* attribute's collation, if any */
     Oid            attcollation BKI_LOOKUP_OPT(pg_collation);

-    /*
-     * compression method.  Must be InvalidCompressionMethod if and only if
-     * typstorage is 'plain' or 'external'.
-     */
-    char        attcompression BKI_DEFAULT('\0');
-
 #ifdef CATALOG_VARLEN            /* variable-length fields start here */
     /* NOTE: The following fields are not present in tuple descriptors. */

@@ -190,10 +190,10 @@ CATALOG(pg_attribute,1249,AttributeRelationId) BKI_BOOTSTRAP BKI_ROWTYPE_OID(75,
  * ATTRIBUTE_FIXED_PART_SIZE is the size of the fixed-layout,
  * guaranteed-not-null part of a pg_attribute row.  This is in fact as much
  * of the row as gets copied into tuple descriptors, so don't expect you
- * can access fields beyond attcollation except in a real tuple!
+ * can access the variable-length fields except in a real tuple!
  */
 #define ATTRIBUTE_FIXED_PART_SIZE \
-    (offsetof(FormData_pg_attribute,attcompression) + sizeof(char))
+    (offsetof(FormData_pg_attribute,attcollation) + sizeof(Oid))

 /* ----------------
  *        Form_pg_attribute corresponds to a pointer to a tuple with