Re: Extend postgres_fdw_get_connections to return remote backend pid

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