> On May 22, 2026, at 21:42, Fujii Masao <[email protected]> wrote: > > On Fri, May 22, 2026 at 8:33 PM Rafia Sabih <[email protected]> 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/
v4-0001-Set-notice-receiver-before-libpq-connection-start.patch
Description: Binary data
