Re: pg_subscription.subslotname is wrongly marked NOT NULL

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: pgsql-hackers(at)lists(dot)postgresql(dot)org
Subject: Re: pg_subscription.subslotname is wrongly marked NOT NULL
Date: 2020-07-20 20:53:59
Message-ID: 732838.1595278439@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: 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

Attachment Content-Type Size
backpatchable-not-null-fixes.patch text/x-diff 4.3 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Mark Dilger 2020-07-20 21:02:28 Re: new heapcheck contrib module
Previous Message Andres Freund 2020-07-20 20:30:46 Re: Allow ERROR from heap_prepare_freeze_tuple to be downgraded to WARNING