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

Mopping this up ... the attached patch against v12 shows the portions
of 72eab84a5 and 0fa0b487b that I'm thinking of putting into v10-v12.

The doc changes, which just clarify that subslotname and srsublsn can
be null, should be uncontroversial.  The changes in pg_subscription.c
prevent it from accessing data that might not be there.  99.999% of
the time, that doesn't matter; we'd copy garbage into
SubscriptionRelState.lsn, but the callers shouldn't look at that field
in states where it's not valid.  However, it's possible that the code
could access data off the end of the heap page, and at least in theory
that could lead to a SIGSEGV.

What I'm not quite sure about is whether to add the BKI_FORCE_NULL
annotations to the headers or not.  There are some pros and cons:

* While Catalog.pm has had support for BKI_FORCE_NULL for quite some
time, we never used it in anger before yesterday.  It's easy to
check that it works, but I wonder whether anybody has third-party
analysis tools that look at the catalog headers and would get broken
because they didn't cover this.

* If we change these markings, then our own testing in the buildfarm
etc. will not reflect the state of affairs seen in many/most actual
v10-v12 installations.  The scope of code where it'd matter seems
pretty tiny, so I don't think there's a big risk, but there's more
than zero risk.  (In any case, I would not push this part until all
the buildfarm JIT critters have reported happiness with 798b4faef,
as that's the one specific spot where it surely does matter.)

* On the other side of the ledger, if we don't fix these markings
we cannot back-patch the additional assertions I proposed at [1].

I'm kind of leaning to committing this as shown and back-patching
the patch at [1], but certainly a case could be made in the other
direction.  Thoughts?

            regards, tom lane

[1] https://www.postgresql.org/message-id/298837.1595196283%40sss.pgh.pa.us

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 76ed2fbea8..582f6f66c6 100644
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6786,8 +6786,9 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
       <entry><structfield>subslotname</structfield></entry>
       <entry><type>name</type></entry>
       <entry></entry>
-      <entry>Name of the replication slot in the upstream database. Also used
-       for local replication origin name.</entry>
+      <entry>Name of the replication slot in the upstream database (also used
+       for the local replication origin name);
+       null represents <literal>NONE</literal></entry>
      </row>

      <row>
@@ -6869,7 +6870,9 @@ SCRAM-SHA-256$<replaceable><iteration count></replaceable>:<replaceable>&l
       <entry><type>pg_lsn</type></entry>
       <entry></entry>
       <entry>
-       End LSN for <literal>s</literal> and <literal>r</literal> states.
+       Remote LSN of the state change used for synchronization coordination
+       when in <literal>s</literal> or <literal>r</literal> states,
+       otherwise null
       </entry>
      </row>
     </tbody>
diff --git a/src/backend/catalog/pg_subscription.c b/src/backend/catalog/pg_subscription.c
index afee2838cc..a3a8b4e599 100644
--- a/src/backend/catalog/pg_subscription.c
+++ b/src/backend/catalog/pg_subscription.c
@@ -457,13 +457,20 @@ GetSubscriptionRelations(Oid subid)
     {
         Form_pg_subscription_rel subrel;
         SubscriptionRelState *relstate;
+        Datum        d;
+        bool        isnull;

         subrel = (Form_pg_subscription_rel) GETSTRUCT(tup);

         relstate = (SubscriptionRelState *) palloc(sizeof(SubscriptionRelState));
         relstate->relid = subrel->srrelid;
         relstate->state = subrel->srsubstate;
-        relstate->lsn = subrel->srsublsn;
+        d = SysCacheGetAttr(SUBSCRIPTIONRELMAP, tup,
+                            Anum_pg_subscription_rel_srsublsn, &isnull);
+        if (isnull)
+            relstate->lsn = InvalidXLogRecPtr;
+        else
+            relstate->lsn = DatumGetLSN(d);

         res = lappend(res, relstate);
     }
@@ -509,13 +516,20 @@ GetSubscriptionNotReadyRelations(Oid subid)
     {
         Form_pg_subscription_rel subrel;
         SubscriptionRelState *relstate;
+        Datum        d;
+        bool        isnull;

         subrel = (Form_pg_subscription_rel) GETSTRUCT(tup);

         relstate = (SubscriptionRelState *) palloc(sizeof(SubscriptionRelState));
         relstate->relid = subrel->srrelid;
         relstate->state = subrel->srsubstate;
-        relstate->lsn = subrel->srsublsn;
+        d = SysCacheGetAttr(SUBSCRIPTIONRELMAP, tup,
+                            Anum_pg_subscription_rel_srsublsn, &isnull);
+        if (isnull)
+            relstate->lsn = InvalidXLogRecPtr;
+        else
+            relstate->lsn = DatumGetLSN(d);

         res = lappend(res, relstate);
     }
diff --git a/src/include/catalog/pg_subscription.h b/src/include/catalog/pg_subscription.h
index 3cb13d897e..796c8794ca 100644
--- a/src/include/catalog/pg_subscription.h
+++ b/src/include/catalog/pg_subscription.h
@@ -53,7 +53,7 @@ CATALOG(pg_subscription,6100,SubscriptionRelationId) BKI_SHARED_RELATION BKI_ROW
     text        subconninfo BKI_FORCE_NOT_NULL;

     /* Slot name on publisher */
-    NameData    subslotname;
+    NameData    subslotname BKI_FORCE_NULL;

     /* Synchronous commit setting for worker */
     text        subsynccommit BKI_FORCE_NOT_NULL;
diff --git a/src/include/catalog/pg_subscription_rel.h b/src/include/catalog/pg_subscription_rel.h
index f7df814a18..6f5607d604 100644
--- a/src/include/catalog/pg_subscription_rel.h
+++ b/src/include/catalog/pg_subscription_rel.h
@@ -34,8 +34,18 @@ CATALOG(pg_subscription_rel,6102,SubscriptionRelRelationId)
     Oid            srsubid;        /* Oid of subscription */
     Oid            srrelid;        /* Oid of relation */
     char        srsubstate;        /* state of the relation in subscription */
-    XLogRecPtr    srsublsn;        /* remote lsn of the state change used for
-                                 * synchronization coordination */
+
+    /*
+     * Although srsublsn is a fixed-width type, it is allowed to be NULL, so
+     * we prevent direct C code access to it just as for a varlena field.
+     */
+#ifdef CATALOG_VARLEN            /* variable-length fields start here */
+
+    XLogRecPtr    srsublsn BKI_FORCE_NULL;    /* remote LSN of the state change
+                                             * used for synchronization
+                                             * coordination, or NULL if not
+                                             * valid */
+#endif
 } FormData_pg_subscription_rel;

 typedef FormData_pg_subscription_rel *Form_pg_subscription_rel;