Re: Set notice receiver before libpq connection startup

Re: Set notice receiver before libpq connection startup

> On May 22, 2026, at 23:28, Fujii Masao  wrote:
> 
> On Fri, May 22, 2026 at 11:56 PM Chao Li  wrote:
>> PFA v4, just removed conn NULL check.
> 
> Thanks for updating the patch! I've pushed it.
> 
> Regards,
> 
> -- 
> Fujii Masao

Thanks for pushing and still working hard during the PGConf.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Re: Set notice receiver before libpq connection startup

> On May 22, 2026, at 21:42, Fujii Masao  wrote:
> 
> On Fri, May 22, 2026 at 8:33 PM Rafia Sabih  wrote:
>> 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.
> 
> Yes, but I'm fine with the current code in the patch. That code makes
> the intent explicit, i.e., install the notice receiver only when a connection
> object actually exists. That said, I'm also OK with simply calling
> PQsetNoticeReceiver() without that check.
> 

Every PG**() function checks if conn is NULL, so I am okay to remove the check.

> 
>> 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.
> 
> Not only connect_pg_server() but libpqsrv_connect_complete() has
> a PG_TRY/PG_CATCH block. So if start_conn were not used, an error thrown
> in libpqsrv_connect_complete() could cause the current connection (conn) to
> be cleaned up twice unexpectedly: once in libpqsrv_connect_complete() and
> again in connect_pg_server(). I guess that's why Chao introduced start_conn.
> 

Exactly. With introducing start_conn, when libpqsrv_connect_complete() raises an error, conn is still NULL, so that PG_CATCH clause won’t cleanup conn, which keeps the same behavior as the old code.

PFA v4, just removed conn NULL check.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

BUG #19570: redundant double negation prevents IN-subquery pull-up and causes a slower SubPlan

The following bug has been logged on the website:

Bug reference:      19570
Logged by:          cl hl
Email address:      2320415112@qq.com
PostgreSQL version: 17.10
Operating system:   Linux LAPTOP-2SQAVLB0 6.6.87.2-microsoft-standard-
Description:        

## Description

This issue concerns the identity `NOT NOT P = P`, which is valid under SQL
three-valued logic because double negation preserves TRUE, FALSE, and NULL.
When `P` is an `IN` subquery, PostgreSQL chooses different plans for the
equivalent forms.

### Expected Behaviour

PostgreSQL should remove double negation before subquery planning and
produce the same semijoin plan as the unwrapped `IN` predicate.

### Actual Behaviour

The plain predicate is pulled up into a `Hash Semi Join`. The double-negated
form remains a hashed `SubPlan` evaluated by an outer sequential scan. In
the standalone case, execution time increases from 7.143 ms to 12.321 ms,
approximately 1.72x.

Generated pair 3137 exhibits a larger order-stable instance: median
execution time increases from 2.255 ms to 404.978 ms, approximately 178.6x.

## How to repeat

```sql
DROP TABLE IF EXISTS identity_outer;
DROP TABLE IF EXISTS identity_inner;

CREATE TABLE identity_outer (v INTEGER NOT NULL);
CREATE TABLE identity_inner (v INTEGER NOT NULL);

INSERT INTO identity_outer
SELECT g FROM generate_series(90001, 91000) AS g;

INSERT INTO identity_inner
SELECT g FROM generate_series(1, 100000) AS g;

ANALYZE identity_outer;
ANALYZE identity_inner;

-- Original form P.
EXPLAIN (ANALYZE, BUFFERS, VERBOSE, SETTINGS, TIMING OFF)
SELECT COUNT(*)
FROM identity_outer AS o
WHERE o.v IN (
    SELECT i.v FROM identity_inner AS i
);

-- Equivalent double-negated form NOT NOT P.
EXPLAIN (ANALYZE, BUFFERS, VERBOSE, SETTINGS, TIMING OFF)
SELECT COUNT(*)
FROM identity_outer AS o
WHERE NOT NOT (
    o.v IN (SELECT i.v FROM identity_inner AS i)
);
```

Both queries return 1,000. Characteristic plans and measured times are:

```text
P:
  Hash Semi Join
  Execution Time: 7.143 ms

NOT NOT P:
  Seq Scan on identity_outer
    Filter: ANY (... hashed SubPlan 1 ...)
  Execution Time: 12.321 ms
```

## Assessment

Re: Set notice receiver before libpq connection startup

On 23.05.26 01:56, Chao Li wrote:
> 
> 
>> On May 22, 2026, at 23:28, Fujii Masao  wrote:
>>
>> On Fri, May 22, 2026 at 11:56 PM Chao Li  wrote:
>>> PFA v4, just removed conn NULL check.
>>
>> Thanks for updating the patch! I've pushed it.
>>
>> Regards,
>>
>> -- 
>> Fujii Masao
> 
> Thanks for pushing and still working hard during the PGConf.

The committed patch violates the .gitattributes whitespace rules:

git show --check 06a5c3cdef02

contrib/postgres_fdw/connection.c:651: indent with spaces.
+                                                   /* expand_dbname = 
*/ false);

It is unfortunate that pgindent produces this layout that contradicts 
the git configuration.  (The current formatting also doesn't look like 
what I would produce in an editor, so I think git is right here.)

Maybe we could reformat this slightly to avoid that?  (unless someone 
wants to try to fix pgindent)

Re: Set notice receiver before libpq connection startup

> On May 28, 2026, at 13:22, Fujii Masao  wrote:
> 
> On Thu, May 28, 2026 at 9:40 AM Chao Li  wrote:
>>> The committed patch violates the .gitattributes whitespace rules:
>>> 
>>> git show --check 06a5c3cdef02
>>> 
>>> contrib/postgres_fdw/connection.c:651: indent with spaces.
>>> +                                                   /* expand_dbname = */ false);
>>> 
>>> It is unfortunate that pgindent produces this layout that contradicts the git configuration.  (The current formatting also doesn't look like what I would produce in an editor, so I think git is right here.)
>>> 
>>> Maybe we could reformat this slightly to avoid that?  (unless someone wants to try to fix pgindent)
> 
> Thanks for the report!
> 
> 
>> Ah, I was not aware of the whitespace rule. I think it was not pgindent; I made that change manually. I added “=” because I thought it might read more fluently.
>> 
>> Attached is a fix for that.
> 
> Thanks for the patch!
> 
> I suspect the whitespace issue was caused by pgindent that I ran
> before committing the patch.
> 
> -     /* expand_dbname = */ false);
> +    false /* expand_dbname */ );
> 
> I think "/* expand_dbname = */ false" looks better. libpqwalreceiver.c
> also uses that comment style. So how about the attached v2 patch, which
> reformats the comment accordingly?
> 
> After applying the v2 patch, I confirmed that neither "git show --check ..."
> nor pgindent reports any issues.
> 
> Regards,
> 
> -- 
> Fujii Masao
> 

Oh, I misunderstood the problem. Nice to learn a new thing, I never knew git show check before. Yes, v2 passed “git show --check d359d02a238”.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Re: Set notice receiver before libpq connection startup

> On May 20, 2026, at 17:19, Fujii Masao  wrote:
> 
> On Wed, May 20, 2026 at 4:21 PM Chao Li  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.

Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/

Re: Set notice receiver before libpq connection startup

Re: Set notice receiver before libpq connection startup

On Thu, May 28, 2026 at 2:47 PM Chao Li  wrote:
> Yes, v2 passed “git show --check d359d02a238”.

I've pushed v2 patch. Thanks!

Regards,

-- 
Fujii Masao

Re: Set notice receiver before libpq connection startup

On Thu, May 28, 2026 at 9:40 AM Chao Li  wrote:
> > The committed patch violates the .gitattributes whitespace rules:
> >
> > git show --check 06a5c3cdef02
> >
> > contrib/postgres_fdw/connection.c:651: indent with spaces.
> > +                                                   /* expand_dbname = */ false);
> >
> > It is unfortunate that pgindent produces this layout that contradicts the git configuration.  (The current formatting also doesn't look like what I would produce in an editor, so I think git is right here.)
> >
> > Maybe we could reformat this slightly to avoid that?  (unless someone wants to try to fix pgindent)

Thanks for the report!


> Ah, I was not aware of the whitespace rule. I think it was not pgindent; I made that change manually. I added “=” because I thought it might read more fluently.
>
> Attached is a fix for that.

Thanks for the patch!

I suspect the whitespace issue was caused by pgindent that I ran
before committing the patch.

-     /* expand_dbname = */ false);
+    false /* expand_dbname */ );

I think "/* expand_dbname = */ false" looks better. libpqwalreceiver.c
also uses that comment style. So how about the attached v2 patch, which
reformats the comment accordingly?

After applying the v2 patch, I confirmed that neither "git show --check ..."
nor pgindent reports any issues.

Regards,

-- 
Fujii Masao

Re: Set notice receiver before libpq connection startup

On Wed, May 20, 2026 at 4:21 PM Chao Li  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

Re: Set notice receiver before libpq connection startup

On Fri, May 22, 2026 at 8:33 PM Rafia Sabih  wrote:
> 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.

Yes, but I'm fine with the current code in the patch. That code makes
the intent explicit, i.e., install the notice receiver only when a connection
object actually exists. That said, I'm also OK with simply calling
PQsetNoticeReceiver() without that check.


> 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.

Not only connect_pg_server() but libpqsrv_connect_complete() has
a PG_TRY/PG_CATCH block. So if start_conn were not used, an error thrown
in libpqsrv_connect_complete() could cause the current connection (conn) to
be cleaned up twice unexpectedly: once in libpqsrv_connect_complete() and
again in connect_pg_server(). I guess that's why Chao introduced start_conn.

Regards,

-- 
Fujii Masao

Re: Set notice receiver before libpq connection startup

On Thu, 21 May 2026 at 07:40, Chao Li  wrote:
>
>
>
> > On May 20, 2026, at 17:19, Fujii Masao  wrote:
> >
> > On Wed, May 20, 2026 at 4:21 PM Chao Li  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