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
Date: Sun, 19 Jul 2020 18:04:43 -0400
Message-ID: <298837.1595196283@sss.pgh.pa.us> (raw)
In-Reply-To: <34984.1595124581@sss.pgh.pa.us>
References: <4118109.1595096139@sss.pgh.pa.us>
<34984.1595124581@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
Attachments:add-attnotnull-assertions.patch (text/x-diff, 2.5K) downloaddiff --git a/src/backend/catalog/indexing.c b/src/backend/catalog/indexing.cindex 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);
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, pgsql-hackers@lists.postgresql.org, andres@anarazel.de
Subject: Re: pg_subscription.subslotname is wrongly marked NOT NULL
In-Reply-To: <298837.1595196283@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox