From: Tom Lane <tgl@sss.pgh.pa.us>
To: Jeff Davis <pgsql@j-davis.com>
cc: Amit Kapila <amit.kapila16@gmail.com>,
        Shlok Kyal <shlok.kyal.oss@gmail.com>, Noah Misch <noah@leadboat.com>,
        pgsql-hackers@postgresql.org
Subject: Re: CREATE SUBSCRIPTION ... SERVER vs. pg_dump, etc.
In-reply-to: <95a4a0d1f9b1a2e9d2e339b41b532131571150af.camel@j-davis.com>
References: <20260710195902.4f.noahmisch@microsoft.com>
 <65960c946d048687a254501dcfedf23956968620.camel@j-davis.com>
 <CAA4eK1KEnEAW8UWEwxf5UGxE7Qttf3=AnobuxVZUr9e9TiQnqg@mail.gmail.com>
 <d7d168cb94fb5543fd603a4deb79e4a253a5725a.camel@j-davis.com>
 <CANhcyEWRTw6-eD94q2e=MuwUYDqpkmaVQsOK8ZE1L2t+NjoDFg@mail.gmail.com>
 <CAA4eK1LpnhyRA2-CuHTPQZG_kXnhi8PegEHdinv=iEQ-pHZYaQ@mail.gmail.com>
 <95a4a0d1f9b1a2e9d2e339b41b532131571150af.camel@j-davis.com>
Comments: In-reply-to Jeff Davis <pgsql@j-davis.com>
	message dated "Wed, 05 Aug 2026 19:18:02 -0700"
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1787285.1786328153.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable
Date: Sun, 09 Aug 2026 22:15:53 -0400
Message-ID: <1787286.1786328153@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1787286.1786328153%40sss.pgh.pa.us>
Precedence: bulk

Jeff Davis <pgsql@j-davis.com> writes:
> On Wed, 2026-08-05 at 16:47 +0530, Amit Kapila wrote:
>> BTW, I had also looked at the overall patch series, the idea and
>> high-level code looks good to me. Though I haven't done a detailed
>> testing or review of the same but as Shlok and Kuroda-San seem to
>> have
>> reviewed/tested these patches, I think we can go-ahead with these
>> fixes.

> Thank you all, pushed.

Coverity thinks there is a hole in this logic:

/srv/coverity/git/pgsql-git/postgresql/src/backend/commands/subscriptioncm=
ds.c: 875             in CreateSubscription()
869     	values[Anum_pg_subscription_submaxretention - 1] =3D
870     		Int32GetDatum(opts.maxretention);
871     	values[Anum_pg_subscription_subretentionactive - 1] =3D
872     		BoolGetDatum(opts.retaindeadtuples);
873     	values[Anum_pg_subscription_subserver - 1] =3D ObjectIdGetDatum(s=
erverid);
874     	if (!OidIsValid(serverid))
>>>     CID 1699896:         Null pointer dereferences  (FORWARD_NULL)
>>>     Passing null pointer "conninfo" to "cstring_to_text", which derefe=
rences it.
875     		values[Anum_pg_subscription_subconninfo - 1] =3D
876     			CStringGetTextDatum(conninfo);
877     	else
878     		nulls[Anum_pg_subscription_subconninfo - 1] =3D true;
879     	if (opts.slot_name)
880     		values[Anum_pg_subscription_subslotname - 1] =3D

AFAICS, it's right: if stmt->servername is set while opts.connect is
not, we'll arrive at this step with serverid filled in but conninfo
still NULL.  Even if there's some upstream reason why that combination
can't occur, this is pretty fragile-looking code.  It's far from clear
why serverid has anything to do with conninfo being available.

			regards, tom lane