Re: Extend postgres_fdw_get_connections to return remote backend pid

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(dot)fujii(at)oss(dot)nttdata(dot)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(dot)fujii(at)oss(dot)nttdata(dot)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