Re: pg_subscription.subslotname is wrongly marked NOT NULL - Mailing list pgsql-hackers

I wrote:
>> It's also a bit annoying that we have no mechanized checks that
>> would catch this inconsistency.  If JIT is going to be absolutely
>> dependent on NOT NULL markings being accurate, we can't really
>> have such a laissez-faire attitude to C code getting it wrong.

> It seems like at least in assert-enabled builds, we'd better have
> a cross-check for that.  I'm not sure where's the best place.

I concluded that we should put this into CatalogTupleInsert and
CatalogTupleUpdate.  The bootstrap data path already has a check
(see InsertOneNull()), and so does the executor, so we only need
to worry about tuples that're built manually by catalog manipulation
code.  I think all of that goes through these functions.  Hence,
as attached.

... and apparently, I should have done this task first, because
damn if it didn't immediately expose another bug of the same ilk.
pg_subscription_rel.srsublsn also needs to be marked nullable.

            regards, tom lane

diff --git a/src/backend/catalog/indexing.c b/src/backend/catalog/indexing.c
index d63fcf58cf..fe277f3ad3 100644
--- 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);