Horton

Re: pg_subscription.subslotname is wrongly marked NOT NULL

Tom Lane <tgl@sss.pgh.pa.us>

View thread View on postgr.es
From: Tom Lane <tgl@sss.pgh.pa.us>
To: pgsql-hackers@lists.postgresql.org
Date: 2020-07-20T20:53:59Z
Lists: pgsql-hackers

Attachments

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

Commits

  1. Assert that we don't insert nulls into attnotnull catalog columns.

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

  3. Kluge slot_compile_deform() to ignore incorrect attnotnull markings.

  4. Correctly mark pg_subscription_rel.srsublsn as nullable.

  5. Correctly mark pg_subscription.subslotname as nullable.