Thread: ps command does not show walsender's connected db
Hi, When walsender process is evoked for logical replication, walsender is connected to a database of the subscriber. Naturally, ones would want the name of the connected database to show in the entry of ps command for walsender. In detail, running ps aux during the logical replication shows results like the following: postgres=# \! ps aux | grep postgres; ... ACTC-I\+ 14575 0.0 0.0 298620 14228 ? Ss 18:22 0:00 postgres: walsender ACTC-I\nakamorit [local] S However, since walsender is connected to a database of the subscriber in logical replication, it should show the database name, as in the following: postgres=# \! ps aux | grep postgres ... ACTC-I\+ 15627 0.0 0.0 298624 13936 ? Ss 15:45 0:00 postgres: walsender ACTC-I\nakamorit postgres Showing the database name should not apply in streaming replication though since walsender process is not connected to any specific database. The attached patch adds the name of the connected database to the ps entry of walsender in logical replication, and not in streaming replication. Thoughts? Tatsuhiro Nakamori
Attachment
On 2022/10/06 22:30, bt22nakamorit wrote:
> Hi,
>
> When walsender process is evoked for logical replication, walsender is connected to a database of the subscriber.
> Naturally, ones would want the name of the connected database to show in the entry of ps command for walsender.
> In detail, running ps aux during the logical replication shows results like the following:
> postgres=# \! ps aux | grep postgres;
> ...
> ACTC-I\+ 14575 0.0 0.0 298620 14228 ? Ss 18:22 0:00 postgres: walsender ACTC-I\nakamorit [local] S
>
> However, since walsender is connected to a database of the subscriber in logical replication,
s/subscriber/publisher ?
> it should show the database name, as in the following:
> postgres=# \! ps aux | grep postgres
> ...
> ACTC-I\+ 15627 0.0 0.0 298624 13936 ? Ss 15:45 0:00 postgres: walsender ACTC-I\nakamorit postgres
>
> Showing the database name should not apply in streaming replication though since walsender process is not connected
toany specific database.
>
> The attached patch adds the name of the connected database to the ps entry of walsender in logical replication, and
notin streaming replication.
>
> Thoughts?
+1
Thanks for the patch!
-
+ printf("fork child process\n");
+ printf(" am_walsender: %d\n", am_walsender);
+ printf(" am_db_walsender: %d\n", am_db_walsender);
The patch seems to include the debug code accidentally.
Except this, the patch looks good to me.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
2022-10-07 16:59 Fujii Masao wrote:
> s/subscriber/publisher ?
I did not understand what this means.
> Thanks for the patch!
>
> -
> + printf("fork child process\n");
> + printf(" am_walsender: %d\n", am_walsender);
> + printf(" am_db_walsender: %d\n", am_db_walsender);
>
> The patch seems to include the debug code accidentally.
> Except this, the patch looks good to me.
I apologize for the silly mistake.
I edited the patch and attached it to this mail.
Best,
Tatsuhiro Nakamori
Attachment
On 2022/10/07 18:43, bt22nakamorit wrote:
> 2022-10-07 16:59 Fujii Masao wrote:
>
>> s/subscriber/publisher ?
>
> I did not understand what this means.
Sorry for confusing wording... I was just trying to say that walsender
is connected to a database of the publisher instead of subscriber.
>> Thanks for the patch!
>>
>> -
>> + printf("fork child process\n");
>> + printf(" am_walsender: %d\n", am_walsender);
>> + printf(" am_db_walsender: %d\n", am_db_walsender);
>>
>> The patch seems to include the debug code accidentally.
>> Except this, the patch looks good to me.
>
> I apologize for the silly mistake.
> I edited the patch and attached it to this mail.
Thanks for updating the patch! LGTM.
Regards,
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
On Fri, Oct 7, 2022 at 7:25 PM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:
>
> Thanks for updating the patch! LGTM.
- if (!am_walsender)
+ if (!am_walsender || am_db_walsender)
appendStringInfo(&ps_data, "%s ", port->database_name);
Can the appendStringInfo be just unconditional? That is more readable
IMO. We want the database_name to be appended whenever it isn't null.
The only case we expect database_name to be null is for walsenders
serving streaming replication standbys and even when database_name is
null, nothing gets appended, it's just the appendStringInfo() call
gets wasted, but that's okay.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
2022-10-09 18:30 Bharath Rupireddy wrote:
> - if (!am_walsender)
> + if (!am_walsender || am_db_walsender)
> appendStringInfo(&ps_data, "%s ", port->database_name);
>
> Can the appendStringInfo be just unconditional? That is more readable
> IMO. We want the database_name to be appended whenever it isn't null.
I agree that the patch makes the code less neat.
> The only case we expect database_name to be null is for walsenders
> serving streaming replication standbys and even when database_name is
> null, nothing gets appended, it's just the appendStringInfo() call
> gets wasted, but that's okay.
appendStringInfo will append extra space after null (since "%s "), so
the ps entry will look less neat in that case.
How about we check whether port->database_name is null or not, instead
of making it unconditional?
It will look like this.
- if (!am_walsender)
+ if (port->database_name != NULL)
appendStringInfo(&ps_data, "%s ", port->database_name);
Thank you for your response,
Tatsuhiro Nakamori
On Mon, Oct 10, 2022 at 8:00 AM bt22nakamorit
<bt22nakamorit@oss.nttdata.com> wrote:
>
> appendStringInfo will append extra space after null (since "%s "), so
> the ps entry will look less neat in that case.
> How about we check whether port->database_name is null or not, instead
> of making it unconditional?
> It will look like this.
> - if (!am_walsender)
> + if (port->database_name != NULL)
> appendStringInfo(&ps_data, "%s ", port->database_name);
if (port->database_name != NULL && port->database_name[0] != '\0')
appendStringInfo(&ps_data, "%s ", port->database_name);
The above works better.
--
Bharath Rupireddy
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
2022-10-10 12:27 Bharath Rupireddy wrote: > if (port->database_name != NULL && port->database_name[0] != '\0') > appendStringInfo(&ps_data, "%s ", port->database_name); > > The above works better. Thank you for the suggestion. I have edited the patch to reflect your idea. Best, Tatsuhiro Nakamori
Attachment
On Mon, Oct 10, 2022 at 12:20 PM bt22nakamorit <bt22nakamorit@oss.nttdata.com> wrote: > > 2022-10-10 12:27 Bharath Rupireddy wrote: > > if (port->database_name != NULL && port->database_name[0] != '\0') > > appendStringInfo(&ps_data, "%s ", port->database_name); > > > > The above works better. > > Thank you for the suggestion. > I have edited the patch to reflect your idea. Thanks. LGTM. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
2022-10-10 16:12 Bharath Rupireddy wrote: > Thanks. LGTM. Thank you for your review! I have this issue posted on Commitfest 2022-11 with title "show walsender's connected db for ps command entry". May I change the status to "Ready for Committer"? Best, Tatsuhiro Nakamori
On Tue, Oct 11, 2022 at 2:11 PM bt22nakamorit <bt22nakamorit@oss.nttdata.com> wrote: > > 2022-10-10 16:12 Bharath Rupireddy wrote: > > Thanks. LGTM. > Thank you for your review! > I have this issue posted on Commitfest 2022-11 with title "show > walsender's connected db for ps command entry". > May I change the status to "Ready for Committer"? Done - https://commitfest.postgresql.org/40/3937/. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
2022年10月11日(火) 20:06 Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>: > > On Tue, Oct 11, 2022 at 2:11 PM bt22nakamorit > <bt22nakamorit@oss.nttdata.com> wrote: > > > > 2022-10-10 16:12 Bharath Rupireddy wrote: > > > Thanks. LGTM. > > Thank you for your review! > > I have this issue posted on Commitfest 2022-11 with title "show > > walsender's connected db for ps command entry". > > May I change the status to "Ready for Committer"? > > Done - https://commitfest.postgresql.org/40/3937/. Hi Fujii-san is marked as committer on the commifest entry for this patch [1]; are you able to go ahead and get it committed? [1] https://commitfest.postgresql.org/40/3937/ Thanks Ian Barwick
On Thu, Nov 17, 2022 at 01:32:11PM +0900, Ian Lawrence Barwick wrote: > Fujii-san is marked as committer on the commifest entry for this patch [1]; > are you able to go ahead and get it committed? That's the state of the patch since the 11th of October. Seeing the lack of activity, I propose to take care of that myself if there are no objections, only on HEAD. That could be qualified as a bug, but that does not seem worth bothering the back-branches, either. Except if somebody has a different opinion? The patch looks to do the job correctly, still I would leave the extra printf() calls in BackendStartup() out. -- Michael
Attachment
On Tue, Nov 22, 2022 at 6:14 AM Michael Paquier <michael@paquier.xyz> wrote: > > On Thu, Nov 17, 2022 at 01:32:11PM +0900, Ian Lawrence Barwick wrote: > > Fujii-san is marked as committer on the commifest entry for this patch [1]; > > are you able to go ahead and get it committed? > > That's the state of the patch since the 11th of October. Seeing the > lack of activity, I propose to take care of that myself if there are > no objections, only on HEAD. That could be qualified as a bug, but > that does not seem worth bothering the back-branches, either. Except > if somebody has a different opinion? -1 for back-patching as it's not something critical to any of the server's operations. > The patch looks to do the job correctly, still I would leave the extra > printf() calls in BackendStartup() out. Are you looking at the latest v3 patch https://www.postgresql.org/message-id/4b5691462b994c18ff370aaa84cef0d0%40oss.nttdata.com? It has no printf() calls. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Tue, Nov 22, 2022 at 08:46:22AM +0530, Bharath Rupireddy wrote: > Are you looking at the latest v3 patch > https://www.postgresql.org/message-id/4b5691462b994c18ff370aaa84cef0d0%40oss.nttdata.com? > It has no printf() calls. Yes, I was looking at v1. v3 can be simpler. All this information in the Port comes from the startup packet, and the database name would default to the user name so it can never be NULL. And applied. -- Michael