public inbox for pgsql-hackers@postgresql.org
help / color / mirror / Atom feedFrom: Rafia Sabih <rafia.pghackers@gmail.com>
To: Chao Li <li.evan.chao@gmail.com>
Cc: vignesh C <vignesh21@gmail.com>
Cc: Fujii Masao <masao.fujii@gmail.com>
Cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: Set notice receiver before libpq connection startup
Date: Fri, 22 May 2026 13:33:21 +0200
Message-ID: <CA+FpmFdfyBhuiou1uqe94UR3WCg57wM5FsooUx2w27aABGoyag@mail.gmail.com> (raw)
In-Reply-To: <6B9A85F9-B632-4286-98AF-9EC435019055@gmail.com>
References: <A2B8B7DE-C119-492F-A9FA-14CF86849777@gmail.com>
<CAHGQGwHnz7FoSx+qQsAHBx_j8Tks+SC-kAPTevrKYfrL08Ee9Q@mail.gmail.com>
<978D8971-08C3-4AAD-AE8B-976D753C882A@gmail.com>
<CALDaNm3UyoQ91gW2bRP4PXnnfAv3GfmCik614-AtQCiEUAjQYw@mail.gmail.com>
<6B9A85F9-B632-4286-98AF-9EC435019055@gmail.com>
On Thu, 21 May 2026 at 09:27, Chao Li <li.evan.chao@gmail.com> wrote:
>
>
> > On May 21, 2026, at 13:03, vignesh C <vignesh21@gmail.com> wrote:
> >
> > 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 PM Chao Li <li.evan.chao@gmail.com>
> wrote:
> >>>>
> >>>> Hi,
> >>>>
> >>>> While testing “Log remote NOTICE, WARNING, and similar messages using
> ereport()”, 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 still 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=# create extension dblink;
> >>>> CREATE EXTENSION
> >>>> evantest=# SELECT dblink_connect('host=127.0.0.1 port=5432
> dbname=remotedb user=chaol sslmode=disable gssencmode=disable');
> >>>> 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=127.0.0.1 port=5432 dbname=remotedb user=chaol
> sslmode=disable gssencmode=disable');
> >>>> 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: libpqsrv_connect_with_notice_receiver() and
> libpqsrv_connect_params_with_notice_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=127.0.0.1 port=5432 dbname=remotedb user=chaol
> sslmode=disable gssencmode=disable');
> >>>> ```
> >>>>
> >>>> 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
> performs
> >>> 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
> without
> >>> 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
> connection
> >>> completion. It could also support other kinds of per-connection setup
> in 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_internal(), I just renamed libpqsrv_connect_internal() to
> libpqsrv_connect_complete().
> >> * 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 call ReleaseExternalFD(). I mentioned this in the header
> comment of libpqsrv_connect_start().
> >> * In the postgres_fdw/connection.c change, I introduced a new local
> variable, start_conn, to keep the original logic unchanged. Because there
> is a PG_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 = NULL;
> > +
> > + libpqsrv_connect_prepare();
> > +
> > + conn = 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 = NULL;
> >
> > libpqsrv_connect_prepare();
> >
> > - conn = PQconnectStart(conninfo);
> > -
> > - libpqsrv_connect_internal(conn, wait_event_info);
> > + conn = PQconnectStartParams(keywords, values, expand_dbname);
> >
> > return conn;
> > }
> >
> > Regards,
> > Vignesh
>
> Thanks for your comment. Addressed in v3.
>
> Here are my two cents,
we need not to check conn is null here
+ conn = libpqsrv_connect_start(connstr);
+ if (conn != NULL)
+ PQsetNoticeReceiver(conn, libpqsrv_notice_receiver,
+ "received message via remote connection");
because it is done so in PQsetNoticeReceiver anyway. Also, since there is
no else here so it doesn't make sense more, because if it is null then also
we will just continue with the next function call.
Another point is, in pg_connect_server I don't get the value of adding
another PGConn variable start_conn, can't we use conn itself...?
I hope this helps.
--
Regards,
Rafia Sabih
CYBERTEC PostgreSQL International GmbH
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: rafia.pghackers@gmail.com, li.evan.chao@gmail.com, vignesh21@gmail.com, masao.fujii@gmail.com
Subject: Re: Set notice receiver before libpq connection startup
In-Reply-To: <CA+FpmFdfyBhuiou1uqe94UR3WCg57wM5FsooUx2w27aABGoyag@mail.gmail.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox