Thread: Re: Extend postgres_fdw_get_connections to return remote backend pid
On 2025/02/18 23:44, Sagar Shedge wrote: > Dear Hackers, > > I want to propose to extend existing postgres_fdw_get_connections > functionality to return remote server backend pid. > Using postgres_fdw extension, backend can establish connections to > remote postgres servers. Recently we added functionality to get > connection status which can help users to detect closed connections > immediately. But currently there is no way to get a remote backend PID > for these connections. I assume you're planning to extend postgres_fdw_get_connections() to also return the result of PQbackendPID(entry->conn). However, the patch you attached doesn't seem to include that change. Did you attach the wrong patch? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
On 2025/02/19 2:06, Sagar Shedge wrote:
> Hi Fujii,
>
> On Tue, Feb 18, 2025 at 10:25 PM Fujii Masao
> <masao.fujii@oss.nttdata.com> wrote:
>
>>
>> I assume you're planning to extend postgres_fdw_get_connections() to
>> also return the result of PQbackendPID(entry->conn).
>> However, the patch you attached doesn't seem to include that change.
>> Did you attach the wrong patch?
>
> My bad!!
> You are right. I was going through an old discussion and attached the
> same old patch file.
>
> Please refer to the patch file to return the remote backend pid.
Thanks for the patch!
Here are my review comments:
The documentation needs to be updated.
+ OUT closed boolean, OUT remote_backend_pid INTEGER)
Naming is always tricky, but remote_backend_pid feels a bit too long.
Would remote_pid be sufficient?
* For API version 1.2 and later, this function takes an input parameter
* to check a connection status and returns the following
* additional values along with the three values from version 1.1:
"three values" should be changed to "four values".
+ * - remote_backend_pid - return remote server backend pid
For consistency with other field comments, "return" seems unnecessary.
How about: "PID of the remote backend handling the connection" instead?
+ /*
+ * If a connection status is not closed and remote backend
+ * ID is valid, return remote backend ID. Otherwise, return NULL.
+ */
+ remote_backend_pid = PQbackendPID(entry->conn);
+ if ((is_conn_closed != 1) && (remote_backend_pid != 0))
+ values[i++] = remote_backend_pid;
Wouldn't it be better to return the result of PQbackendPID() instead of NULL
even when the connection is closed, for debugging purposes? This way,
users can see which remote backend previously handled the "closed" connection,
which might be helpful for troubleshooting.
The postgres_fdw regression test failed on my MacBook with the following diff:
--- /Users/postgres/pgsql/git/contrib/postgres_fdw/expected/postgres_fdw.out 2025-02-19 12:53:27
+++ /Users/postgres/pgsql/git/contrib/postgres_fdw/results/postgres_fdw.out 2025-02-19 17:40:04
@@ -12443,7 +12443,7 @@
FROM postgres_fdw_get_connections(true);
server_name | remote_conn_closed | remote_backend_pid
-------------+--------------------+--------------------
- loopback | false | available
+ loopback | true | available
(1 row)
-- After terminating the remote backend, since the connection is closed,
@@ -12458,7 +12458,7 @@
FROM postgres_fdw_get_connections(true);
server_name | remote_conn_closed | remote_backend_pid
-------------+--------------------+--------------------
- loopback | true | not available
+ loopback | true | available
(1 row)
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Hi Fujii, > Naming is always tricky, but remote_backend_pid feels a bit too long. Would remote_pid be sufficient? Point looks valid. I had another perspective is to align the naming convention to pg_backend_pid(). remote_pid is not helping to identify whether pid belongs to postgres backend or not. Does this make sense? Or I'm fine to go with concise name like `remote_pid` > "three values" should be changed to "four values". Done. Good catch!! > How about: "PID of the remote backend handling the connection" instead? Updated in v2. > Wouldn't it be better to return the result of PQbackendPID() instead of NULL even when the connection is closed, for debugging purposes? This way, users can see which remote backend previously handled the "closed" connection, which might be helpful for troubleshooting. Agree. Updated logic to return backend pid always except when pid is 0 which indicates no backend attached to session. Returning 0 found misleading. What's your thought on this? > The postgres_fdw regression test failed on my MacBook with the following diff: I updated tests to make it more stable. Let me know if it's still failing on your setup. On Wed, Feb 19, 2025 at 2:19 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2025/02/19 2:06, Sagar Shedge wrote: > > Hi Fujii, > > > > On Tue, Feb 18, 2025 at 10:25 PM Fujii Masao > > <masao.fujii@oss.nttdata.com> wrote: > > > >> > >> I assume you're planning to extend postgres_fdw_get_connections() to > >> also return the result of PQbackendPID(entry->conn). > >> However, the patch you attached doesn't seem to include that change. > >> Did you attach the wrong patch? > > > > My bad!! > > You are right. I was going through an old discussion and attached the > > same old patch file. > > > > Please refer to the patch file to return the remote backend pid. > > Thanks for the patch! > > Here are my review comments: > > The documentation needs to be updated. > > > + OUT closed boolean, OUT remote_backend_pid INTEGER) > > Naming is always tricky, but remote_backend_pid feels a bit too long. > Would remote_pid be sufficient? > > > * For API version 1.2 and later, this function takes an input parameter > * to check a connection status and returns the following > * additional values along with the three values from version 1.1: > > "three values" should be changed to "four values". > > > + * - remote_backend_pid - return remote server backend pid > > For consistency with other field comments, "return" seems unnecessary. > How about: "PID of the remote backend handling the connection" instead? > > > + /* > + * If a connection status is not closed and remote backend > + * ID is valid, return remote backend ID. Otherwise, return NULL. > + */ > + remote_backend_pid = PQbackendPID(entry->conn); > + if ((is_conn_closed != 1) && (remote_backend_pid != 0)) > + values[i++] = remote_backend_pid; > > Wouldn't it be better to return the result of PQbackendPID() instead of NULL > even when the connection is closed, for debugging purposes? This way, > users can see which remote backend previously handled the "closed" connection, > which might be helpful for troubleshooting. > > > The postgres_fdw regression test failed on my MacBook with the following diff: > > --- /Users/postgres/pgsql/git/contrib/postgres_fdw/expected/postgres_fdw.out 2025-02-19 12:53:27 > +++ /Users/postgres/pgsql/git/contrib/postgres_fdw/results/postgres_fdw.out 2025-02-19 17:40:04 > @@ -12443,7 +12443,7 @@ > FROM postgres_fdw_get_connections(true); > server_name | remote_conn_closed | remote_backend_pid > -------------+--------------------+-------------------- > - loopback | false | available > + loopback | true | available > (1 row) > > -- After terminating the remote backend, since the connection is closed, > @@ -12458,7 +12458,7 @@ > FROM postgres_fdw_get_connections(true); > server_name | remote_conn_closed | remote_backend_pid > -------------+--------------------+-------------------- > - loopback | true | not available > + loopback | true | available > (1 row) > > > Regards, > > -- > Fujii Masao > Advanced Computing Technology Center > Research and Development Headquarters > NTT DATA CORPORATION > -- Sagar Dilip Shedge, SDE AWS
Attachment
On 2025/02/21 0:54, Sagar Shedge wrote:
> Hi Fujii,
>
>> Naming is always tricky, but remote_backend_pid feels a bit too long.
> Would remote_pid be sufficient?
> Point looks valid. I had another perspective is to align the naming
> convention to pg_backend_pid(). remote_pid is not helping to identify
> whether pid belongs to postgres backend or not. Does this make sense?
> Or I'm fine to go with concise name like `remote_pid`
I initially thought "remote_pid" was sufficient since the postgres_fdw
connection clearly corresponds to a backend process. However, I'm fine
with keeping "remote_backend_pid" as the column name for now. If we find
a better name later, we can rename it.
>> How about: "PID of the remote backend handling the connection" instead?
> Updated in v2.
Thanks for updating the patch!
You still need to update the documentation. Could you add descriptions
for postgres_fdw_get_connections()?
>> Wouldn't it be better to return the result of PQbackendPID() instead of NULL
> even when the connection is closed, for debugging purposes? This way,
> users can see which remote backend previously handled the "closed" connection,
> which might be helpful for troubleshooting.
> Agree. Updated logic to return backend pid always except when pid is 0
> which indicates no backend attached to session. Returning 0 found
> misleading. What's your thought on this?
Your approach makes sense to me.
>> The postgres_fdw regression test failed on my MacBook with the following diff:
> I updated tests to make it more stable. Let me know if it's still
> failing on your setup.
Yes, the regression test passed successfully on my machine.
--- dropped.
-SELECT server_name, user_name = CURRENT_USER as "user_name = CURRENT_USER", valid, used_in_xact, closed
+-- dropped. remote_backend_pid will continue to return available as it fetch remote
+-- server backend pid from cached connections.
+SELECT server_name, user_name = CURRENT_USER as "user_name = CURRENT_USER", valid, used_in_xact, closed,
+CASE WHEN remote_backend_pid IS NOT NULL then 'available' ELSE 'not available' END AS remote_backend_pid
Instead of checking whether remote_backend_pid is NOT NULL, how about
verifying that it actually matches a remote backend's PID? For example:
remote_backend_pid = ANY(SELECT pid FROM pg_stat_activity WHERE backend_type = 'client backend' AND pid <>
pg_backend_pid())AS "remote_backend_pid = remote pg_stat_activity.pid"
-SELECT CASE WHEN closed IS NOT true THEN 1 ELSE 0 END
+SELECT server_name, CASE WHEN closed IS NOT true THEN 'false' ELSE 'true' END AS remote_conn_closed,
+CASE WHEN remote_backend_pid IS NOT NULL then 'available' ELSE 'not available' END AS remote_backend_pid
Similarly, instead of checking if remote_backend_pid is NOT NULL,
how about verifying it against pg_stat_activity?
remote_backend_pid = (SELECT pid FROM pg_stat_activity WHERE application_name = 'fdw_conn_check')
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
Hi Fujii, Please review the latest patch. > Could you add descriptions for postgres_fdw_get_connections()? Done. > how about verifying it against pg_stat_activity? Yes. This approach will make tests more reliable. Updated. On Fri, Feb 21, 2025 at 8:15 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2025/02/21 0:54, Sagar Shedge wrote: > > Hi Fujii, > > > >> Naming is always tricky, but remote_backend_pid feels a bit too long. > > Would remote_pid be sufficient? > > Point looks valid. I had another perspective is to align the naming > > convention to pg_backend_pid(). remote_pid is not helping to identify > > whether pid belongs to postgres backend or not. Does this make sense? > > Or I'm fine to go with concise name like `remote_pid` > > I initially thought "remote_pid" was sufficient since the postgres_fdw > connection clearly corresponds to a backend process. However, I'm fine > with keeping "remote_backend_pid" as the column name for now. If we find > a better name later, we can rename it. > > > >> How about: "PID of the remote backend handling the connection" instead? > > Updated in v2. > > Thanks for updating the patch! > > You still need to update the documentation. Could you add descriptions > for postgres_fdw_get_connections()? > > > >> Wouldn't it be better to return the result of PQbackendPID() instead of NULL > > even when the connection is closed, for debugging purposes? This way, > > users can see which remote backend previously handled the "closed" connection, > > which might be helpful for troubleshooting. > > Agree. Updated logic to return backend pid always except when pid is 0 > > which indicates no backend attached to session. Returning 0 found > > misleading. What's your thought on this? > > Your approach makes sense to me. > > > >> The postgres_fdw regression test failed on my MacBook with the following diff: > > I updated tests to make it more stable. Let me know if it's still > > failing on your setup. > > Yes, the regression test passed successfully on my machine. > > > --- dropped. > -SELECT server_name, user_name = CURRENT_USER as "user_name = CURRENT_USER", valid, used_in_xact, closed > +-- dropped. remote_backend_pid will continue to return available as it fetch remote > +-- server backend pid from cached connections. > +SELECT server_name, user_name = CURRENT_USER as "user_name = CURRENT_USER", valid, used_in_xact, closed, > +CASE WHEN remote_backend_pid IS NOT NULL then 'available' ELSE 'not available' END AS remote_backend_pid > > Instead of checking whether remote_backend_pid is NOT NULL, how about > verifying that it actually matches a remote backend's PID? For example: > > remote_backend_pid = ANY(SELECT pid FROM pg_stat_activity WHERE backend_type = 'client backend' AND pid <> pg_backend_pid())AS "remote_backend_pid = remote pg_stat_activity.pid" > > > -SELECT CASE WHEN closed IS NOT true THEN 1 ELSE 0 END > +SELECT server_name, CASE WHEN closed IS NOT true THEN 'false' ELSE 'true' END AS remote_conn_closed, > +CASE WHEN remote_backend_pid IS NOT NULL then 'available' ELSE 'not available' END AS remote_backend_pid > > Similarly, instead of checking if remote_backend_pid is NOT NULL, > how about verifying it against pg_stat_activity? > > remote_backend_pid = (SELECT pid FROM pg_stat_activity WHERE application_name = 'fdw_conn_check') > > Regards, > > -- > Fujii Masao > Advanced Computing Technology Center > Research and Development Headquarters > NTT DATA CORPORATION > -- Sagar Dilip Shedge, SDE AWS
Attachment
On 2025/02/21 22:43, Sagar Shedge wrote: > Hi Fujii, > > Please review the latest patch. Thanks for updating the patch! >> Could you add descriptions for postgres_fdw_get_connections()? > Done. Thanks! But, I realize my previous comment may have been unclear. I was referring to updating doc/src/sgml/postgres-fdw.sgml, which contains the explanation of postgres_fdw_get_connections(). Could you update that? Regards, -- Fujii Masao Advanced Computing Technology Center Research and Development Headquarters NTT DATA CORPORATION
Hi Fujii, > Could you update that? I apologize. Updated documentation. Please review and let me know if you have suggestions to improve wording. On Fri, Feb 21, 2025 at 7:56 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote: > > > > On 2025/02/21 22:43, Sagar Shedge wrote: > > Hi Fujii, > > > > Please review the latest patch. > > Thanks for updating the patch! > > > >> Could you add descriptions for postgres_fdw_get_connections()? > > Done. > > Thanks! But, I realize my previous comment may have been unclear. > I was referring to updating doc/src/sgml/postgres-fdw.sgml, > which contains the explanation of postgres_fdw_get_connections(). > Could you update that? > > Regards, > > -- > Fujii Masao > Advanced Computing Technology Center > Research and Development Headquarters > NTT DATA CORPORATION > -- Sagar Dilip Shedge, Pune. With Regards.