MIME-Version: 1.0
References: <A2B8B7DE-C119-492F-A9FA-14CF86849777@gmail.com>
 <CAHGQGwHnz7FoSx+qQsAHBx_j8Tks+SC-kAPTevrKYfrL08Ee9Q@mail.gmail.com>
 <978D8971-08C3-4AAD-AE8B-976D753C882A@gmail.com>
In-Reply-To: <978D8971-08C3-4AAD-AE8B-976D753C882A@gmail.com>
From: vignesh C <vignesh21@gmail.com>
Date: Thu, 21 May 2026 10:33:09 +0530
Message-ID: 
 <CALDaNm3UyoQ91gW2bRP4PXnnfAv3GfmCik614-AtQCiEUAjQYw@mail.gmail.com>
Subject: Re: Set notice receiver before libpq connection startup
To: Chao Li <li.evan.chao@gmail.com>
Cc: Fujii Masao <masao.fujii@gmail.com>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CALDaNm3UyoQ91gW2bRP4PXnnfAv3GfmCik614-AtQCiEUAjQYw%40mail.gmail.com>
Precedence: bulk

On Thu, 21 May 2026 at 07:40, Chao Li <li.evan.chao@gmail.com> wrote:
>
>
>
> > On May 20, 2026, at 17:19, Fujii Masao <masao.fujii@gmail.com> wrote:
> >
> > On Wed, May 20, 2026 at 4:21=E2=80=AFPM Chao Li <li.evan.chao@gmail.com=
> wrote:
> >>
> >> Hi,
> >>
> >> While testing =E2=80=9CLog remote NOTICE, WARNING, and similar message=
s using ereport()=E2=80=9D, I noticed that libpqsrv_notice_receiver is only=
 installed after libpqsrv_connect() finishes. As a result, NOTICE messages =
generated during connection establishment are missed by ereport() and are s=
till printed to stderr.
> >>
> >> To reproduce the issue, I created a separate database called remotedb =
and defined a login trigger that emits a NOTICE message:
> >> ```
> >> CREATE DATABASE remotedb;
> >>
> >> \c remotedb
> >>
> >> CREATE OR REPLACE FUNCTION repro_login_notice()
> >> RETURNS event_trigger
> >> LANGUAGE plpgsql AS $$
> >> BEGIN
> >>  RAISE NOTICE 'startup notice from remotedb login trigger';
> >> END;
> >> $$;
> >>
> >> CREATE EVENT TRIGGER repro_login_notice_trg
> >> ON login
> >> EXECUTE FUNCTION repro_login_notice();
> >>
> >> ALTER EVENT TRIGGER repro_login_notice_trg ENABLE ALWAYS;
> >> ```
> >>
> >> Then, from another database:
> >> ```
> >> evantest=3D# create extension dblink;
> >> CREATE EXTENSION
> >> evantest=3D# SELECT dblink_connect('host=3D127.0.0.1 port=3D5432 dbnam=
e=3Dremotedb user=3Dchaol sslmode=3Ddisable gssencmode=3Ddisable');
> >> dblink_connect
> >> ----------------
> >> OK
> >> (1 row)
> >> ```
> >>
> >> In the system log, the NOTICE message is printed directly:
> >> ```
> >> 2026-05-20 13:02:19.350 CST [24909] STATEMENT:  SELECT dblink_connect(=
'host=3D127.0.0.1 port=3D5432 dbname=3Dremotedb user=3Dchaol sslmode=3Ddisa=
ble gssencmode=3Ddisable');
> >> NOTICE:  startup notice from remotedb login trigger
> >> ```
> >>
> >> To fix that, I think we should install libpqsrv_notice_receiver before=
 libpqsrv_connect_internal(). In the attached patch, I added two helpers: l=
ibpqsrv_connect_with_notice_receiver() and libpqsrv_connect_params_with_not=
ice_receiver().
> >>
> >> With the fix, the NOTICE message now looks like this:
> >> ```
> >> 2026-05-20 14:44:49.296 CST [45567] LOG:  received message via remote =
connection: NOTICE:  startup notice from remotedb login trigger
> >> 2026-05-20 14:44:49.296 CST [45567] STATEMENT:  SELECT dblink_connect(=
'host=3D127.0.0.1 port=3D5432 dbname=3Dremotedb user=3Dchaol sslmode=3Ddisa=
ble gssencmode=3Ddisable');
> >> ```
> >>
> >> Please see the attached patch for details.
> >
> > Thanks for the report and patch!
> >
> > I'd prefer to avoid adding notice-receiver-specific wrappers such as
> > libpqsrv_connect_with_notice_receiver(). Instead, how about splitting
> > libpqsrv_connect() into two steps: libpqsrv_connect_start(), which perf=
orms
> > libpqsrv_connect_prepare() and PQconnectStart(), and
> > libpqsrv_connect_complete(), which runs libpqsrv_connect_internal()?
> >
> > With this approach, callers could invoke PQsetNoticeReceiver() after
> > libpqsrv_connect_start() returns but before libpqsrv_connect_complete()=
 is
> > called. This would allow startup-time notices to be handled correctly w=
ithout
> > introducing a dedicated wrapper function.
> >
> > Compared to the *_with_notice_receiver() approach, this design is more
> > general because it exposes the phase between PQconnectStart() and conne=
ction
> > completion. It could also support other kinds of per-connection setup i=
n the
> > future, not just notice receiver installation. Thought?
> >
> > Regards,
> >
> > --
> > Fujii Masao
>
> The idea sounds good to me, so v2 is implemented following that idea.
>
> A few things I want to point out abut v2:
>
> * Since libpqsrv_connect_complete() would only wrap libpqsrv_connect_inte=
rnal(), I just renamed libpqsrv_connect_internal() to libpqsrv_connect_comp=
lete().
> * Since libpqsrv_connect() is now split into two phases, libpqsrv_connect=
_complete() must be called even if conn is NULL, because it may need to cal=
l ReleaseExternalFD(). I mentioned this in the header comment of libpqsrv_c=
onnect_start().
> * In the postgres_fdw/connection.c change, I introduced a new local varia=
ble, start_conn, to keep the original logic unchanged. Because there is a P=
G_TRY/PG_CATCH block, conn is assigned only after libpqsrv_connect_complete=
() finishes successfully.

Thanks for reporting this issue. I was able to reproduce the issue
with the steps provided and your patch fixes the issue.
Few comments:
1) No need of conn variable here, we can directly return
PQconnectStart(conninfo) in this function:
+static inline PGconn *
+libpqsrv_connect_start(const char *conninfo)
+{
+ PGconn    *conn =3D NULL;
+
+ libpqsrv_connect_prepare();
+
+ conn =3D PQconnectStart(conninfo);
+
+ return conn;
+}

2) Similarly here too:
+static inline PGconn *
+libpqsrv_connect_params_start(const char *const *keywords,
+   const char *const *values,
+   int expand_dbname)
 {
  PGconn    *conn =3D NULL;

  libpqsrv_connect_prepare();

- conn =3D PQconnectStart(conninfo);
-
- libpqsrv_connect_internal(conn, wait_event_info);
+ conn =3D PQconnectStartParams(keywords, values, expand_dbname);

  return conn;
 }

Regards,
Vignesh