Horton

Thread

Commits

  1. Add the database name to the ps display of logical WAL senders

  1. ps command does not show walsender's connected db

    bt22nakamorit <bt22nakamorit@oss.nttdata.com> — 2022-10-06T13:30:56Z

    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

  2. Re: ps command does not show walsender's connected db

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2022-10-07T07:59:48Z

    
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 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?

+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

  3. Re: ps command does not show walsender's connected db

    bt22nakamorit <bt22nakamorit@oss.nttdata.com> — 2022-10-07T09:43:51Z

    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

  4. Re: ps command does not show walsender's connected db

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2022-10-07T13:55:39Z

    
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

  5. Re: ps command does not show walsender's connected db

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-10-09T09:30:49Z

    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

  6. Re: ps command does not show walsender's connected db

    bt22nakamorit <bt22nakamorit@oss.nttdata.com> — 2022-10-10T02:30:03Z

    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

  7. Re: ps command does not show walsender's connected db

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-10-10T03:27:31Z

    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

  8. Re: ps command does not show walsender's connected db

    bt22nakamorit <bt22nakamorit@oss.nttdata.com> — 2022-10-10T06:50:21Z

    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

  9. Re: ps command does not show walsender's connected db

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-10-10T07:12:29Z

    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

  10. Re: ps command does not show walsender's connected db

    bt22nakamorit <bt22nakamorit@oss.nttdata.com> — 2022-10-11T08:41:51Z

    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

  11. Re: ps command does not show walsender's connected db

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-10-11T11:06:34Z

    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

  12. Re: ps command does not show walsender's connected db

    Ian Lawrence Barwick <barwick@gmail.com> — 2022-11-17T04:32:11Z

    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

  13. Re: ps command does not show walsender's connected db

    Michael Paquier <michael@paquier.xyz> — 2022-11-22T00:44:43Z

    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

  14. Re: ps command does not show walsender's connected db

    Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> — 2022-11-22T03:16:22Z

    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

  15. Re: ps command does not show walsender's connected db

    Michael Paquier <michael@paquier.xyz> — 2022-11-24T07:20:55Z

    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