Re: Gripes about walsender command processing

Michael Paquier <michael(at)paquier(dot)xyz> writes:
> On Sun, Sep 13, 2020 at 03:47:51PM -0400, Tom Lane wrote:
>> While trying to fix this, I also observed that exec_replication_command
>> fails to clean up the temp context it made for parsing the command string,
>> if that turns out to be a SQL command. This very accidentally fails to
>> lead to a long-term memory leak, because that context will be a child of
>> MessageContext so it'll be cleaned out in the next iteration of
>> PostgresMain's main loop. But it's still unacceptably sloppy coding
>> IMHO, and it's closely related to a lot of other randomness in the
>> function; it'd be better to have a separate early-exit path for SQL
>> commands.

> Looks fine seen from here. +1.

Pushed, thanks for looking.

>> What I'd really like to do is adjust pgstat_report_activity so that
>> callers are *required* to provide some kind of command string when
>> transitioning into STATE_RUNNING state, ie something like
>> Assert(state == STATE_RUNNING ? cmd_str != NULL : cmd_str == NULL);

> For this one, I don't actually agree. For now the state and the query
> string, actually the activity string, are completely independent. So
> external modules like bgworkers can mix the state enum and the
> activity string as they wish. I think that this flexibility is useful
> to keep.

Meh ... but I'm not excited enough about the point to argue.

I looked into the other point about ps status always claiming the
walsender is "idle". This turns out to be something PostgresMain
does automatically, so unless we want to uglify that logic, we have
to make walsenders set the field honestly. So I propose the attached.
(Is there a better way to get the name of a replication command?
I didn't look very hard for one.)

regards, tom lane