Assert that we don't insert nulls into attnotnull catalog columns.

The executor checks for this error, and so does the bootstrap catalog
loader, but we never checked for it in retail catalog manipulations.
The folly of that has now been exposed, so let's add assertions
checking it.  Checking in CatalogTupleInsert[WithInfo] and
CatalogTupleUpdate[WithInfo] should be enough to cover this.

Back-patch to v10; the aforesaid functions didn't exist before that,
and it didn't seem worth adapting the patch to the oldest branches.
But given the risk of JIT crashes, I think we certainly need this
as far back as v11.

Pre-v13, we have to explicitly exclude pg_subscription.subslotname
and pg_subscription_rel.srsublsn from the checks, since they are
mismarked.  (Even if we change our mind about applying BKI_FORCE_NULL
in the branch tips, it doesn't seem wise to have assertions that
would fire in existing databases.)

Discussion: https://postgr.es/m/298837.1595196283@sss.pgh.pa.us

diff --git a/doc/src/sgml/bki.sgml b/doc/src/sgml/bki.sgml
index 6776c4a3c18661f756a5f1b0bcdc2c485758498a..4e7568f5ce9283d0d470e64a4ac176e3bb7c56e2 100644 (file)
--- a/doc/src/sgml/bki.sgml
+++ b/doc/src/sgml/bki.sgml
@@ -122,10 +122,7 @@
    if they are fixed-width and are not preceded by any nullable column.
    Where this rule is inadequate, you can force correct marking by using
    <literal>BKI_FORCE_NOT_NULL</literal>
-   and <literal>BKI_FORCE_NULL</literal> annotations as needed.  But note
-   that <literal>NOT NULL</literal> constraints are only enforced in the
-   executor, not against tuples that are generated by random C code,
-   so care is still needed when manually creating or updating catalog rows.
+   and <literal>BKI_FORCE_NULL</literal> annotations as needed.
   </para>
 
   <para>

diff --git a/src/backend/catalog/indexing.c b/src/backend/catalog/indexing.c
index d63fcf58cf1dd3914609aea178b84ff0bd7075bf..fe277f3ad3712d396842c6826a8c85a2e1c00d73 100644 (file)
--- a/src/backend/catalog/indexing.c
+++ b/src/backend/catalog/indexing.c
@@ -167,6 +167,43 @@ CatalogIndexInsert(CatalogIndexState indstate, HeapTuple heapTuple)
    ExecDropSingleTupleTableSlot(slot);
 }
 
+/*
+ * Subroutine to verify that catalog constraints are honored.
+ *
+ * Tuples inserted via CatalogTupleInsert/CatalogTupleUpdate are generally
+ * "hand made", so that it's possible that they fail to satisfy constraints
+ * that would be checked if they were being inserted by the executor.  That's
+ * a coding error, so we only bother to check for it in assert-enabled builds.
+ */
+#ifdef USE_ASSERT_CHECKING
+
+static void
+CatalogTupleCheckConstraints(Relation heapRel, HeapTuple tup)
+{
+   /*
+    * Currently, the only constraints implemented for system catalogs are
+    * attnotnull constraints.
+    */
+   if (HeapTupleHasNulls(tup))
+   {
+       TupleDesc   tupdesc = RelationGetDescr(heapRel);
+       bits8      *bp = tup->t_data->t_bits;
+
+       for (int attnum = 0; attnum < tupdesc->natts; attnum++)
+       {
+           Form_pg_attribute thisatt = TupleDescAttr(tupdesc, attnum);
+
+           Assert(!(thisatt->attnotnull && att_isnull(attnum, bp)));
+       }
+   }
+}
+
+#else                          /* !USE_ASSERT_CHECKING */
+
+#define CatalogTupleCheckConstraints(heapRel, tup)  ((void) 0)
+
+#endif                         /* USE_ASSERT_CHECKING */
+
 /*
  * CatalogTupleInsert - do heap and indexing work for a new catalog tuple
  *
@@ -184,6 +221,8 @@ CatalogTupleInsert(Relation heapRel, HeapTuple tup)
 {
    CatalogIndexState indstate;
 
+   CatalogTupleCheckConstraints(heapRel, tup);
+
    indstate = CatalogOpenIndexes(heapRel);
 
    simple_heap_insert(heapRel, tup);
@@ -204,6 +243,8 @@ void
 CatalogTupleInsertWithInfo(Relation heapRel, HeapTuple tup,
                           CatalogIndexState indstate)
 {
+   CatalogTupleCheckConstraints(heapRel, tup);
+
    simple_heap_insert(heapRel, tup);
 
    CatalogIndexInsert(indstate, tup);
@@ -225,6 +266,8 @@ CatalogTupleUpdate(Relation heapRel, ItemPointer otid, HeapTuple tup)
 {
    CatalogIndexState indstate;
 
+   CatalogTupleCheckConstraints(heapRel, tup);
+
    indstate = CatalogOpenIndexes(heapRel);
 
    simple_heap_update(heapRel, otid, tup);
@@ -245,6 +288,8 @@ void
 CatalogTupleUpdateWithInfo(Relation heapRel, ItemPointer otid, HeapTuple tup,
                           CatalogIndexState indstate)
 {
+   CatalogTupleCheckConstraints(heapRel, tup);
+
    simple_heap_update(heapRel, otid, tup);
 
    CatalogIndexInsert(indstate, tup);