Re: bug when apply fast default mechanism for adding new column over domain with default value - Mailing list pgsql-hackers

jian he <jian.universality@gmail.com> writes:
> On Sun, Mar 2, 2025 at 7:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think the fundamental problem here is that StoreAttrDefault
>> shouldn't be involved in this in the first place.

> i've split missingval related code into StoreAttrMissingVal.

Hm, this does nothing to improve the modularity of the affected
code; if anything it's worse than before.  (Fools around for
awhile...)  I had something more like the attached in mind.
The idea here is to centralize the control of whether we are
storing a missingval or doing a table rewrite in ATExecAddColumn.
StoreAttrMissingVal has nothing to do with pg_attrdef nor does
StoreAttrDefault have anything to do with attmissingval.

I looked briefly at determining the presence of a default
earlier so we could avoid the extra update of pg_attribute
when both those changes need to happen, but I concluded that
it'd take more refactoring than it's worth.  The problem is
the logic way down inside AddRelationNewConstraints that
checks for the eventually-cooked default expression just
being a null constant.

            regards, tom lane

From 97c896cefa8a0ea4203974b40bfed6008213b95a Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sun, 2 Mar 2025 15:34:05 -0500
Subject: [PATCH v3 1/2] Fix broken handling of domains in atthasmissing logic.

If a domain type has a default, adding a column of that type (without
any explicit DEFAULT clause) failed to install the domain's default
value in existing rows, instead leaving the new column null.  This
is unexpected, and it used to work correctly before v11.  The cause
is confusion in the atthasmissing mechanism about which default value
to install: we'd only consider installing an explicitly-specified
default, and then we'd decide that no table rewrite is needed.

To fix, take the responsibility for filling attmissingval out of
StoreAttrDefault, and instead put it into ATExecAddColumn's existing
logic that derives the correct value to fill the new column with.
Also, centralize the logic that determines the need for
default-related table rewriting there, instead of spreading it over
four or five places.

In the back branches, we'll leave the attmissingval-filling code
in StoreAttrDefault even though it's now dead, for fear that some
extension may be depending on that functionality to exist there.
A separate HEAD-only patch will clean up the now-useless code.

Reported-by: jian he <jian.universality@gmail.com>
Author: jian he <jian.universality@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CACJufxHFssPvkP1we7WMhPD_1kwgbG52o=kQgL+TnVoX5LOyCQ@mail.gmail.com
Backpatch-through: 13
---
 src/backend/catalog/heap.c                 | 71 ++++++++++++++++--
 src/backend/catalog/pg_attrdef.c           |  6 ++
 src/backend/commands/tablecmds.c           | 85 +++++++++++++++-------
 src/include/catalog/heap.h                 |  5 +-
 src/test/regress/expected/fast_default.out | 65 +++++++++++++++++
 src/test/regress/sql/fast_default.sql      | 44 +++++++++++
 6 files changed, 242 insertions(+), 34 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 956f196fc95..d28ed3eb396 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -69,6 +69,7 @@
 #include "pgstat.h"
 #include "storage/lmgr.h"
 #include "storage/predicate.h"
+#include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
 #include "utils/inval.h"
@@ -2008,6 +2009,69 @@ RelationClearMissing(Relation rel)
     table_close(attr_rel, RowExclusiveLock);
 }

+/*
+ * StoreAttrMissingVal
+ *
+ * Set the missing value of a single attribute.
+ */
+void
+StoreAttrMissingVal(Relation rel, AttrNumber attnum,
+                    Datum missingval, bool missingIsNull)
+{
+    Datum        valuesAtt[Natts_pg_attribute] = {0};
+    bool        nullsAtt[Natts_pg_attribute] = {0};
+    bool        replacesAtt[Natts_pg_attribute] = {0};
+    Relation    attrrel;
+    Form_pg_attribute attStruct;
+    HeapTuple    atttup,
+                newtup;
+
+    /* This is only supported for plain tables */
+    Assert(rel->rd_rel->relkind == RELKIND_RELATION);
+
+    /* Update the pg_attribute row */
+    attrrel = table_open(AttributeRelationId, RowExclusiveLock);
+
+    atttup = SearchSysCache2(ATTNUM,
+                             ObjectIdGetDatum(RelationGetRelid(rel)),
+                             Int16GetDatum(attnum));
+    if (!HeapTupleIsValid(atttup))    /* shouldn't happen */
+        elog(ERROR, "cache lookup failed for attribute %d of relation %u",
+             attnum, RelationGetRelid(rel));
+    attStruct = (Form_pg_attribute) GETSTRUCT(atttup);
+
+    if (missingIsNull)
+    {
+        /* if the default value is NULL, just store a NULL array */
+        missingval = (Datum) 0;
+    }
+    else
+    {
+        /* otherwise make a one-element array of the value */
+        missingval = PointerGetDatum(construct_array(&missingval,
+                                                     1,
+                                                     attStruct->atttypid,
+                                                     attStruct->attlen,
+                                                     attStruct->attbyval,
+                                                     attStruct->attalign));
+    }
+
+    valuesAtt[Anum_pg_attribute_atthasmissing - 1] = BoolGetDatum(!missingIsNull);
+    replacesAtt[Anum_pg_attribute_atthasmissing - 1] = true;
+
+    valuesAtt[Anum_pg_attribute_attmissingval - 1] = missingval;
+    nullsAtt[Anum_pg_attribute_attmissingval - 1] = missingIsNull;
+    replacesAtt[Anum_pg_attribute_attmissingval - 1] = true;
+
+    newtup = heap_modify_tuple(atttup, RelationGetDescr(attrrel),
+                               valuesAtt, nullsAtt, replacesAtt);
+    CatalogTupleUpdate(attrrel, &newtup->t_self, newtup);
+
+    /* clean up */
+    ReleaseSysCache(atttup);
+    table_close(attrrel, RowExclusiveLock);
+}
+
 /*
  * SetAttrMissing
  *
@@ -2390,13 +2454,8 @@ AddRelationNewConstraints(Relation rel,
              castNode(Const, expr)->constisnull))
             continue;

-        /* If the DEFAULT is volatile we cannot use a missing value */
-        if (colDef->missingMode &&
-            contain_volatile_functions_after_planning((Expr *) expr))
-            colDef->missingMode = false;
-
         defOid = StoreAttrDefault(rel, colDef->attnum, expr, is_internal,
-                                  colDef->missingMode);
+                                  false);

         cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));
         cooked->contype = CONSTR_DEFAULT;
diff --git a/src/backend/catalog/pg_attrdef.c b/src/backend/catalog/pg_attrdef.c
index 91dafc81021..a58815a5134 100644
--- a/src/backend/catalog/pg_attrdef.c
+++ b/src/backend/catalog/pg_attrdef.c
@@ -120,6 +120,12 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
         valuesAtt[Anum_pg_attribute_atthasdef - 1] = true;
         replacesAtt[Anum_pg_attribute_atthasdef - 1] = true;

+        /*
+         * Note: this code is dead so far as core Postgres is concerned,
+         * because no caller passes add_column_mode = true anymore.  We keep
+         * it in back branches on the slight chance that some extension is
+         * depending on it.
+         */
         if (rel->rd_rel->relkind == RELKIND_RELATION && add_column_mode &&
             !attgenerated)
         {
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index ce7d115667e..cd90f2c7367 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -7321,14 +7321,7 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
         rawEnt = (RawColumnDefault *) palloc(sizeof(RawColumnDefault));
         rawEnt->attnum = attribute->attnum;
         rawEnt->raw_default = copyObject(colDef->raw_default);
-
-        /*
-         * Attempt to skip a complete table rewrite by storing the specified
-         * DEFAULT value outside of the heap.  This may be disabled inside
-         * AddRelationNewConstraints if the optimization cannot be applied.
-         */
-        rawEnt->missingMode = (colDef->generated != ATTRIBUTE_GENERATED_STORED);
-
+        rawEnt->missingMode = false;    /* XXX vestigial */
         rawEnt->generated = colDef->generated;

         /*
@@ -7340,13 +7333,6 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,

         /* Make the additional catalog changes visible */
         CommandCounterIncrement();
-
-        /*
-         * Did the request for a missing value work? If not we'll have to do a
-         * rewrite
-         */
-        if (!rawEnt->missingMode)
-            tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
     }

     /*
@@ -7363,9 +7349,9 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
      * rejects nulls.  If there are any domain constraints then we construct
      * an explicit NULL default value that will be passed through
      * CoerceToDomain processing.  (This is a tad inefficient, since it causes
-     * rewriting the table which we really don't have to do, but the present
-     * design of domain processing doesn't offer any simple way of checking
-     * the constraints more directly.)
+     * rewriting the table which we really wouldn't have to do; but we do it
+     * to preserve the historical behavior that such a failure will be raised
+     * only if the table currently contains some rows.)
      *
      * Note: we use build_column_default, and not just the cooked default
      * returned by AddRelationNewConstraints, so that the right thing happens
@@ -7384,6 +7370,9 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
      */
     if (RELKIND_HAS_STORAGE(relkind))
     {
+        bool        has_domain_constraints;
+        bool        has_missing = false;
+
         /*
          * For an identity column, we can't use build_column_default(),
          * because the sequence ownership isn't set yet.  So do it manually.
@@ -7396,14 +7385,13 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
             nve->typeId = attribute->atttypid;

             defval = (Expr *) nve;
-
-            /* must do a rewrite for identity columns */
-            tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
         }
         else
             defval = (Expr *) build_column_default(rel, attribute->attnum);

-        if (!defval && DomainHasConstraints(attribute->atttypid))
+        /* Build CoerceToDomain(NULL) expression if needed */
+        has_domain_constraints = DomainHasConstraints(attribute->atttypid);
+        if (!defval && has_domain_constraints)
         {
             Oid            baseTypeId;
             int32        baseTypeMod;
@@ -7429,18 +7417,61 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
         {
             NewColumnValue *newval;

+            /* Prepare defval for execution, either here or in Phase 3 */
+            defval = expression_planner(defval);
+
+            /* Add the new default to the newvals list */
             newval = (NewColumnValue *) palloc0(sizeof(NewColumnValue));
             newval->attnum = attribute->attnum;
-            newval->expr = expression_planner(defval);
+            newval->expr = defval;
             newval->is_generated = (colDef->generated != '\0');

             tab->newvals = lappend(tab->newvals, newval);
-        }

-        if (DomainHasConstraints(attribute->atttypid))
-            tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
+            /*
+             * Attempt to skip a complete table rewrite by storing the
+             * specified DEFAULT value outside of the heap.  This is only
+             * allowed for plain relations and non-generated columns, and the
+             * default expression can't be volatile (stable is OK).  Note that
+             * contain_volatile_functions deems CoerceToDomain immutable, but
+             * here we consider that coercion to a domain with constraints is
+             * volatile; else it might fail even when the table is empty.
+             */
+            if (rel->rd_rel->relkind == RELKIND_RELATION &&
+                !colDef->generated &&
+                !has_domain_constraints &&
+                !contain_volatile_functions((Node *) defval))
+            {
+                EState       *estate;
+                ExprState  *exprState;
+                Datum        missingval;
+                bool        missingIsNull;
+
+                /* Evaluate the default expression */
+                estate = CreateExecutorState();
+                exprState = ExecPrepareExpr(defval, estate);
+                missingval = ExecEvalExpr(exprState,
+                                          GetPerTupleExprContext(estate),
+                                          &missingIsNull);
+                /* ... and store it.  If it's null, nothing is stored. */
+                StoreAttrMissingVal(rel, attribute->attnum,
+                                    missingval, missingIsNull);
+                has_missing = !missingIsNull;
+                FreeExecutorState(estate);
+            }
+            else
+            {
+                /*
+                 * Failed to use missing mode.  We have to do a table rewrite
+                 * to install the value --- unless it's a virtual generated
+                 * column.
+                 */
+                if (colDef->generated != ATTRIBUTE_GENERATED_VIRTUAL)
+                    tab->rewrite |= AT_REWRITE_DEFAULT_VAL;
+            }
+        }

-        if (!TupleDescAttr(rel->rd_att, attribute->attnum - 1)->atthasmissing)
+        if (!has_missing)
         {
             /*
              * If the new column is NOT NULL, and there is no missing value,
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index 19c594458bd..aa6aaaaa9d8 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -29,7 +29,7 @@ typedef struct RawColumnDefault
 {
     AttrNumber    attnum;            /* attribute to attach default to */
     Node       *raw_default;    /* default value (untransformed parse tree) */
-    bool        missingMode;    /* true if part of add column processing */
+    bool        missingMode;    /* obsolete, no longer used */
     char        generated;        /* attgenerated setting */
 } RawColumnDefault;

@@ -121,6 +121,9 @@ extern List *AddRelationNotNullConstraints(Relation rel,
                                            List *old_notnulls);

 extern void RelationClearMissing(Relation rel);
+
+extern void StoreAttrMissingVal(Relation rel, AttrNumber attnum,
+                                Datum missingval, bool missingIsNull);
 extern void SetAttrMissing(Oid relid, char *attname, char *value);

 extern Node *cookDefault(ParseState *pstate,
diff --git a/src/test/regress/expected/fast_default.out b/src/test/regress/expected/fast_default.out
index 272b57e48cd..ccbcdf8403f 100644
--- a/src/test/regress/expected/fast_default.out
+++ b/src/test/regress/expected/fast_default.out
@@ -257,6 +257,71 @@ SELECT comp();
 (1 row)

 DROP TABLE T;
+-- Test domains with default value for table rewrite.
+CREATE DOMAIN domain1 AS int DEFAULT 11;  -- constant
+CREATE DOMAIN domain2 AS int DEFAULT random(min=>10, max=>100);  -- volatile
+CREATE DOMAIN domain3 AS text DEFAULT foo(4);  -- stable
+CREATE DOMAIN domain4 AS text[]
+  DEFAULT ('{"This", "is", "' || foo(4) || '","the", "real", "world"}')::TEXT[];
+CREATE TABLE t2 (a domain1);
+INSERT INTO t2 VALUES (1),(2);
+-- no table rewrite
+ALTER TABLE t2 ADD COLUMN b domain1 default 3;
+SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
+FROM pg_attribute
+WHERE attnum > 0 AND attrelid = 't2'::regclass
+ORDER BY attnum;
+ attnum | attname | atthasmissing | atthasdef | attmissingval
+--------+---------+---------------+-----------+---------------
+      1 | a       | f             | f         |
+      2 | b       | t             | t         | {3}
+(2 rows)
+
+-- table rewrite should happen
+ALTER TABLE t2 ADD COLUMN c domain3 default left(random()::text,3);
+NOTICE:  rewriting table t2 for reason 2
+-- no table rewrite
+ALTER TABLE t2 ADD COLUMN d domain4;
+SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
+FROM pg_attribute
+WHERE attnum > 0 AND attrelid = 't2'::regclass
+ORDER BY attnum;
+ attnum | attname | atthasmissing | atthasdef |           attmissingval
+--------+---------+---------------+-----------+-----------------------------------
+      1 | a       | f             | f         |
+      2 | b       | f             | t         |
+      3 | c       | f             | t         |
+      4 | d       | t             | f         | {"{This,is,abcd,the,real,world}"}
+(4 rows)
+
+-- table rewrite should happen
+ALTER TABLE t2 ADD COLUMN e domain2;
+NOTICE:  rewriting table t2 for reason 2
+SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
+FROM pg_attribute
+WHERE attnum > 0 AND attrelid = 't2'::regclass
+ORDER BY attnum;
+ attnum | attname | atthasmissing | atthasdef | attmissingval
+--------+---------+---------------+-----------+---------------
+      1 | a       | f             | f         |
+      2 | b       | f             | t         |
+      3 | c       | f             | t         |
+      4 | d       | f             | f         |
+      5 | e       | f             | f         |
+(5 rows)
+
+SELECT a, b, length(c) = 3 as c_ok, d, e >= 10 as e_ok FROM t2;
+ a | b | c_ok |               d               | e_ok
+---+---+------+-------------------------------+------
+ 1 | 3 | t    | {This,is,abcd,the,real,world} | t
+ 2 | 3 | t    | {This,is,abcd,the,real,world} | t
+(2 rows)
+
+DROP TABLE t2;
+DROP DOMAIN domain1;
+DROP DOMAIN domain2;
+DROP DOMAIN domain3;
+DROP DOMAIN domain4;
 DROP FUNCTION foo(INT);
 -- Fall back to full rewrite for volatile expressions
 CREATE TABLE T(pk INT NOT NULL PRIMARY KEY);
diff --git a/src/test/regress/sql/fast_default.sql b/src/test/regress/sql/fast_default.sql
index 6e7f37b17b2..068dd0bc8aa 100644
--- a/src/test/regress/sql/fast_default.sql
+++ b/src/test/regress/sql/fast_default.sql
@@ -248,6 +248,50 @@ SELECT comp();

 DROP TABLE T;

+-- Test domains with default value for table rewrite.
+CREATE DOMAIN domain1 AS int DEFAULT 11;  -- constant
+CREATE DOMAIN domain2 AS int DEFAULT random(min=>10, max=>100);  -- volatile
+CREATE DOMAIN domain3 AS text DEFAULT foo(4);  -- stable
+CREATE DOMAIN domain4 AS text[]
+  DEFAULT ('{"This", "is", "' || foo(4) || '","the", "real", "world"}')::TEXT[];
+
+CREATE TABLE t2 (a domain1);
+INSERT INTO t2 VALUES (1),(2);
+
+-- no table rewrite
+ALTER TABLE t2 ADD COLUMN b domain1 default 3;
+
+SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
+FROM pg_attribute
+WHERE attnum > 0 AND attrelid = 't2'::regclass
+ORDER BY attnum;
+
+-- table rewrite should happen
+ALTER TABLE t2 ADD COLUMN c domain3 default left(random()::text,3);
+
+-- no table rewrite
+ALTER TABLE t2 ADD COLUMN d domain4;
+
+SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
+FROM pg_attribute
+WHERE attnum > 0 AND attrelid = 't2'::regclass
+ORDER BY attnum;
+
+-- table rewrite should happen
+ALTER TABLE t2 ADD COLUMN e domain2;
+
+SELECT attnum, attname, atthasmissing, atthasdef, attmissingval
+FROM pg_attribute
+WHERE attnum > 0 AND attrelid = 't2'::regclass
+ORDER BY attnum;
+
+SELECT a, b, length(c) = 3 as c_ok, d, e >= 10 as e_ok FROM t2;
+
+DROP TABLE t2;
+DROP DOMAIN domain1;
+DROP DOMAIN domain2;
+DROP DOMAIN domain3;
+DROP DOMAIN domain4;
 DROP FUNCTION foo(INT);

 -- Fall back to full rewrite for volatile expressions
--
2.43.5

From 84d5360fec2b598369d68d2c0101812488141adc Mon Sep 17 00:00:00 2001
From: Tom Lane <tgl@sss.pgh.pa.us>
Date: Sun, 2 Mar 2025 16:56:00 -0500
Subject: [PATCH v3 2/2] Remove now-dead code in StoreAttrDefault().

Get rid of RawColumnDefault.missingMode, too, as we no longer
need that to pass information around.

While here, clean up some sloppy coding in StoreAttrDefault(),
such as failure to use XXXGetDatum macros.  These aren't bugs
but they're not good code either.

Reported-by: jian he <jian.universality@gmail.com>
Author: jian he <jian.universality@gmail.com>
Author: Tom Lane <tgl@sss.pgh.pa.us>
Discussion: https://postgr.es/m/CACJufxHFssPvkP1we7WMhPD_1kwgbG52o=kQgL+TnVoX5LOyCQ@mail.gmail.com
---
 src/backend/catalog/heap.c       |  5 +-
 src/backend/catalog/pg_attrdef.c | 79 +++-----------------------------
 src/backend/commands/tablecmds.c |  8 +---
 src/include/catalog/heap.h       |  1 -
 src/include/catalog/pg_attrdef.h |  3 +-
 5 files changed, 12 insertions(+), 84 deletions(-)

diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index d28ed3eb396..1f9250aa065 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -2327,7 +2327,7 @@ StoreConstraints(Relation rel, List *cooked_constraints, bool is_internal)
         {
             case CONSTR_DEFAULT:
                 con->conoid = StoreAttrDefault(rel, con->attnum, con->expr,
-                                               is_internal, false);
+                                               is_internal);
                 break;
             case CONSTR_CHECK:
                 con->conoid =
@@ -2454,8 +2454,7 @@ AddRelationNewConstraints(Relation rel,
              castNode(Const, expr)->constisnull))
             continue;

-        defOid = StoreAttrDefault(rel, colDef->attnum, expr, is_internal,
-                                  false);
+        defOid = StoreAttrDefault(rel, colDef->attnum, expr, is_internal);

         cooked = (CookedConstraint *) palloc(sizeof(CookedConstraint));
         cooked->contype = CONSTR_DEFAULT;
diff --git a/src/backend/catalog/pg_attrdef.c b/src/backend/catalog/pg_attrdef.c
index a58815a5134..1f95bc6f752 100644
--- a/src/backend/catalog/pg_attrdef.c
+++ b/src/backend/catalog/pg_attrdef.c
@@ -14,17 +14,12 @@
  */
 #include "postgres.h"

-#include "access/genam.h"
 #include "access/relation.h"
 #include "access/table.h"
-#include "catalog/catalog.h"
 #include "catalog/dependency.h"
 #include "catalog/indexing.h"
 #include "catalog/objectaccess.h"
 #include "catalog/pg_attrdef.h"
-#include "executor/executor.h"
-#include "optimizer/optimizer.h"
-#include "utils/array.h"
 #include "utils/builtins.h"
 #include "utils/fmgroids.h"
 #include "utils/rel.h"
@@ -35,22 +30,16 @@
  * Store a default expression for column attnum of relation rel.
  *
  * Returns the OID of the new pg_attrdef tuple.
- *
- * add_column_mode must be true if we are storing the default for a new
- * attribute, and false if it's for an already existing attribute. The reason
- * for this is that the missing value must never be updated after it is set,
- * which can only be when a column is added to the table. Otherwise we would
- * in effect be changing existing tuples.
  */
 Oid
 StoreAttrDefault(Relation rel, AttrNumber attnum,
-                 Node *expr, bool is_internal, bool add_column_mode)
+                 Node *expr, bool is_internal)
 {
     char       *adbin;
     Relation    adrel;
     HeapTuple    tuple;
-    Datum        values[4];
-    static bool nulls[4] = {false, false, false, false};
+    Datum        values[Natts_pg_attrdef];
+    static bool nulls[Natts_pg_attrdef] = {false, false, false, false};
     Relation    attrrel;
     HeapTuple    atttup;
     Form_pg_attribute attStruct;
@@ -72,8 +61,8 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
     attrdefOid = GetNewOidWithIndex(adrel, AttrDefaultOidIndexId,
                                     Anum_pg_attrdef_oid);
     values[Anum_pg_attrdef_oid - 1] = ObjectIdGetDatum(attrdefOid);
-    values[Anum_pg_attrdef_adrelid - 1] = RelationGetRelid(rel);
-    values[Anum_pg_attrdef_adnum - 1] = attnum;
+    values[Anum_pg_attrdef_adrelid - 1] = ObjectIdGetDatum(RelationGetRelid(rel));
+    values[Anum_pg_attrdef_adnum - 1] = Int16GetDatum(attnum);
     values[Anum_pg_attrdef_adbin - 1] = CStringGetTextDatum(adbin);

     tuple = heap_form_tuple(adrel->rd_att, values, nulls);
@@ -92,7 +81,7 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,

     /*
      * Update the pg_attribute entry for the column to show that a default
-     * exists.
+     * exists.  (In some cases atthasdef will already be set.)
      */
     attrrel = table_open(AttributeRelationId, RowExclusiveLock);
     atttup = SearchSysCacheCopy2(ATTNUM,
@@ -105,71 +94,17 @@ StoreAttrDefault(Relation rel, AttrNumber attnum,
     attgenerated = attStruct->attgenerated;
     if (!attStruct->atthasdef)
     {
-        Form_pg_attribute defAttStruct;
-
-        ExprState  *exprState;
-        Expr       *expr2 = (Expr *) expr;
-        EState       *estate = NULL;
-        ExprContext *econtext;
         Datum        valuesAtt[Natts_pg_attribute] = {0};
         bool        nullsAtt[Natts_pg_attribute] = {0};
         bool        replacesAtt[Natts_pg_attribute] = {0};
-        Datum        missingval = (Datum) 0;
-        bool        missingIsNull = true;

-        valuesAtt[Anum_pg_attribute_atthasdef - 1] = true;
+        valuesAtt[Anum_pg_attribute_atthasdef - 1] = BoolGetDatum(true);
         replacesAtt[Anum_pg_attribute_atthasdef - 1] = true;

-        /*
-         * Note: this code is dead so far as core Postgres is concerned,
-         * because no caller passes add_column_mode = true anymore.  We keep
-         * it in back branches on the slight chance that some extension is
-         * depending on it.
-         */
-        if (rel->rd_rel->relkind == RELKIND_RELATION && add_column_mode &&
-            !attgenerated)
-        {
-            expr2 = expression_planner(expr2);
-            estate = CreateExecutorState();
-            exprState = ExecPrepareExpr(expr2, estate);
-            econtext = GetPerTupleExprContext(estate);
-
-            missingval = ExecEvalExpr(exprState, econtext,
-                                      &missingIsNull);
-
-            FreeExecutorState(estate);
-
-            defAttStruct = TupleDescAttr(rel->rd_att, attnum - 1);
-
-            if (missingIsNull)
-            {
-                /* if the default evaluates to NULL, just store a NULL array */
-                missingval = (Datum) 0;
-            }
-            else
-            {
-                /* otherwise make a one-element array of the value */
-                missingval = PointerGetDatum(construct_array(&missingval,
-                                                             1,
-                                                             defAttStruct->atttypid,
-                                                             defAttStruct->attlen,
-                                                             defAttStruct->attbyval,
-                                                             defAttStruct->attalign));
-            }
-
-            valuesAtt[Anum_pg_attribute_atthasmissing - 1] = !missingIsNull;
-            replacesAtt[Anum_pg_attribute_atthasmissing - 1] = true;
-            valuesAtt[Anum_pg_attribute_attmissingval - 1] = missingval;
-            replacesAtt[Anum_pg_attribute_attmissingval - 1] = true;
-            nullsAtt[Anum_pg_attribute_attmissingval - 1] = missingIsNull;
-        }
         atttup = heap_modify_tuple(atttup, RelationGetDescr(attrrel),
                                    valuesAtt, nullsAtt, replacesAtt);

         CatalogTupleUpdate(attrrel, &atttup->t_self, atttup);
-
-        if (!missingIsNull)
-            pfree(DatumGetPointer(missingval));
     }
     table_close(attrrel, RowExclusiveLock);
     heap_freetuple(atttup);
diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c
index cd90f2c7367..cf519fb77d9 100644
--- a/src/backend/commands/tablecmds.c
+++ b/src/backend/commands/tablecmds.c
@@ -967,7 +967,6 @@ DefineRelation(CreateStmt *stmt, char relkind, Oid ownerId,
             rawEnt = (RawColumnDefault *) palloc(sizeof(RawColumnDefault));
             rawEnt->attnum = attnum;
             rawEnt->raw_default = colDef->raw_default;
-            rawEnt->missingMode = false;
             rawEnt->generated = colDef->generated;
             rawDefaults = lappend(rawDefaults, rawEnt);
             attr->atthasdef = true;
@@ -7321,7 +7320,6 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel,
         rawEnt = (RawColumnDefault *) palloc(sizeof(RawColumnDefault));
         rawEnt->attnum = attribute->attnum;
         rawEnt->raw_default = copyObject(colDef->raw_default);
-        rawEnt->missingMode = false;    /* XXX vestigial */
         rawEnt->generated = colDef->generated;

         /*
@@ -8075,7 +8073,6 @@ ATExecColumnDefault(Relation rel, const char *colName,
         rawEnt = (RawColumnDefault *) palloc(sizeof(RawColumnDefault));
         rawEnt->attnum = attnum;
         rawEnt->raw_default = newDefault;
-        rawEnt->missingMode = false;
         rawEnt->generated = '\0';

         /*
@@ -8113,7 +8110,7 @@ ATExecCookedColumnDefault(Relation rel, AttrNumber attnum,
     RemoveAttrDefault(RelationGetRelid(rel), attnum, DROP_RESTRICT, false,
                       true);

-    (void) StoreAttrDefault(rel, attnum, newDefault, true, false);
+    (void) StoreAttrDefault(rel, attnum, newDefault, true);

     ObjectAddressSubSet(address, RelationRelationId,
                         RelationGetRelid(rel), attnum);
@@ -8576,7 +8573,6 @@ ATExecSetExpression(AlteredTableInfo *tab, Relation rel, const char *colName,
     rawEnt = (RawColumnDefault *) palloc(sizeof(RawColumnDefault));
     rawEnt->attnum = attnum;
     rawEnt->raw_default = newExpr;
-    rawEnt->missingMode = false;
     rawEnt->generated = attgenerated;

     /* Store the generated expression */
@@ -14128,7 +14124,7 @@ ATExecAlterColumnType(AlteredTableInfo *tab, Relation rel,
         RemoveAttrDefault(RelationGetRelid(rel), attnum, DROP_RESTRICT, true,
                           true);

-        StoreAttrDefault(rel, attnum, defaultexpr, true, false);
+        (void) StoreAttrDefault(rel, attnum, defaultexpr, true);
     }

     ObjectAddressSubSet(address, RelationRelationId,
diff --git a/src/include/catalog/heap.h b/src/include/catalog/heap.h
index aa6aaaaa9d8..d8fbbf68a5b 100644
--- a/src/include/catalog/heap.h
+++ b/src/include/catalog/heap.h
@@ -29,7 +29,6 @@ typedef struct RawColumnDefault
 {
     AttrNumber    attnum;            /* attribute to attach default to */
     Node       *raw_default;    /* default value (untransformed parse tree) */
-    bool        missingMode;    /* obsolete, no longer used */
     char        generated;        /* attgenerated setting */
 } RawColumnDefault;

diff --git a/src/include/catalog/pg_attrdef.h b/src/include/catalog/pg_attrdef.h
index 192799cfed7..3067a8357c0 100644
--- a/src/include/catalog/pg_attrdef.h
+++ b/src/include/catalog/pg_attrdef.h
@@ -57,8 +57,7 @@ DECLARE_FOREIGN_KEY((adrelid, adnum), pg_attribute, (attrelid, attnum));


 extern Oid    StoreAttrDefault(Relation rel, AttrNumber attnum,
-                             Node *expr, bool is_internal,
-                             bool add_column_mode);
+                             Node *expr, bool is_internal);
 extern void RemoveAttrDefault(Oid relid, AttrNumber attnum,
                               DropBehavior behavior,
                               bool complain, bool internal);
--
2.43.5