From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
cc: Andres Freund <andres@anarazel.de>
Subject: Re: pg_subscription.subslotname is wrongly marked NOT NULL
In-reply-to: <34984.1595124581@sss.pgh.pa.us>
References: <4118109.1595096139@sss.pgh.pa.us>
 <34984.1595124581@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Sat, 18 Jul 2020 22:09:41 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <298769.1595196183.0@sss.pgh.pa.us>
Date: Sun, 19 Jul 2020 18:04:43 -0400
Message-ID: <298837.1595196283@sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <298769.1595196183.1@sss.pgh.pa.us>

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


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="add-attnotnull-assertions.patch";
	charset="us-ascii"
Content-ID: <298769.1595196183.2@sss.pgh.pa.us>
Content-Description: add-attnotnull-assertions.patch
Content-Transfer-Encoding: quoted-printable

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, HeapTu=
ple heapTuple)
 	ExecDropSingleTupleTableSlot(slot);
 }
 =

+/*
+ * Subroutine to verify that catalog constraints are honored.
+ *
+ * Tuples inserted via CatalogTupleInsert/CatalogTupleUpdate are generall=
y
+ * "hand made", so that it's possible that they fail to satisfy constrain=
ts
+ * that would be checked if they were being inserted by the executor.  Th=
at's
+ * a coding error, so we only bother to check for it in assert-enabled bu=
ilds.
+ */
+#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 =3D RelationGetDescr(heapRel);
+		bits8	   *bp =3D tup->t_data->t_bits;
+
+		for (int attnum =3D 0; attnum < tupdesc->natts; attnum++)
+		{
+			Form_pg_attribute thisatt =3D 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 =3D 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 =3D 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);

------- =_aaaaaaaaaa0--