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.
> 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);
>
> However, before we could do that, we'd have to clean up the rat's nest
> of seemingly-inserted-with-the-aid-of-a-dartboard pgstat_report_activity
> calls in replication/logical/worker.c. I couldn't make heads or tails
> of what is going on there, so I did not try.
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.
> BTW, while I didn't change it here, isn't the second
> SnapBuildClearExportedSnapshot call in exec_replication_command just
> useless? We already did that before parsing the command.
Indeed.
--
Michael