On Wed, Dec 20, 2017 at 9:02 PM, Magnus Hagander <magnus@hagander.net> wrote: > On Wed, Dec 20, 2017 at 12:50 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: > Yes. Of course. I can't read. That's the same as the notice below about it > not returning false -- I managed to miss the extra if() there, and thought > it always exited with ERROR.
I think that the call to pgstat_report_activity in WalSndLoop() should be kept as well. There is a small gap between the moment the process is started and the first replication command is run.
Eh. But WalSndLoop() is called *after* exec_replication_command(), isn't it? exec_replication_command() is called from PostgresMain(), and then calls WalSndLoop().
So I agree there is a small gap, but actually moving it to exec_replication_command() makes that gap smaller than it was before, no?
>> + /* Report to pgstat that this process is running */ >> + pgstat_report_activity(STATE_RUNNING, NULL); >> Bonus points if cmd_string is used instead of string? This way, you >> can know what is the replication command running ;) > > Do we want that though? That would be a compat break at least, wouldn't it?
Perhaps not, I found the idea funky but you actually don't want to show a string in exec_replication_command for a T_SQLCmd node. That's not complicated to check either. So let's discard such a thing for now.