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);
