Avoid direct C access to possibly-null pg_subscription_rel.srsublsn.

This coding technique is unsafe, since we'd be accessing off the end
of the tuple if the field is null.  SIGSEGV is pretty improbable, but
perhaps not impossible.  Also, returning garbage for the LSN doesn't
seem like a great idea, even if callers aren't looking at it today.

Also update docs to point out explicitly that
pg_subscription.subslotname and pg_subscription_rel.srsublsn
can be null.

Perhaps we should mark these two fields BKI_FORCE_NULL, so that
they'd be correctly labeled in databases that are initdb'd in the
future.  But we can't force that for existing databases, and on
balance it's not too clear that having a mix of different catalog
contents in the field would be wise.

Apply to v10 (where this code came in) through v12.  Already
fixed in v13 and HEAD.

Discussion: https://postgr.es/m/732838.1595278439@sss.pgh.pa.us

diff --git a/doc/src/sgml/catalogs.sgml b/doc/src/sgml/catalogs.sgml
index 7c2cef606c43b30c88373ecffe44844a7ee3f9db..a141576d61bf2e6d1b16025cea94fdd3dd767fdd 100644 (file)
--- a/doc/src/sgml/catalogs.sgml
+++ b/doc/src/sgml/catalogs.sgml
@@ -6695,8 +6695,9 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</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>
@@ -6778,7 +6779,9 @@ SCRAM-SHA-256$<replaceable>&lt;iteration count&gt;</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 8705d8b1d36d536e3ee33266b504a890f3208896..0ce08f7c5b518a9b3078698776fc4b33e34ca06d 100644 (file)
--- 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_rel.h b/src/include/catalog/pg_subscription_rel.h
index 556cb94841dee47adb4185e9b262598436de9425..b907b4468d75f693f287987e2b0652a9cb90b956 100644 (file)
--- a/src/include/catalog/pg_subscription_rel.h
+++ b/src/include/catalog/pg_subscription_rel.h
@@ -34,8 +34,17 @@ CATALOG(pg_subscription_rel,6102,SubscriptionRelRelationId) BKI_WITHOUT_OIDS
        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;               /* 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;