Re: Gripes about walsender command processing - Mailing list pgsql-hackers

Michael Paquier <michael@paquier.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

diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index 67093383e6..a34c5745de 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -1619,19 +1619,23 @@ exec_replication_command(const char *cmd_string)
     switch (cmd_node->type)
     {
         case T_IdentifySystemCmd:
+            set_ps_display("IDENTIFY_SYSTEM");
             IdentifySystem();
             break;

         case T_BaseBackupCmd:
+            set_ps_display("BASE_BACKUP");
             PreventInTransactionBlock(true, "BASE_BACKUP");
             SendBaseBackup((BaseBackupCmd *) cmd_node);
             break;

         case T_CreateReplicationSlotCmd:
+            set_ps_display("CREATE_REPLICATION_SLOT");
             CreateReplicationSlot((CreateReplicationSlotCmd *) cmd_node);
             break;

         case T_DropReplicationSlotCmd:
+            set_ps_display("DROP_REPLICATION_SLOT");
             DropReplicationSlot((DropReplicationSlotCmd *) cmd_node);
             break;

@@ -1639,6 +1643,7 @@ exec_replication_command(const char *cmd_string)
             {
                 StartReplicationCmd *cmd = (StartReplicationCmd *) cmd_node;

+                set_ps_display("START_REPLICATION");
                 PreventInTransactionBlock(true, "START_REPLICATION");

                 if (cmd->kind == REPLICATION_KIND_PHYSICAL)
@@ -1651,6 +1656,7 @@ exec_replication_command(const char *cmd_string)
             }

         case T_TimeLineHistoryCmd:
+            set_ps_display("TIMELINE_HISTORY");
             PreventInTransactionBlock(true, "TIMELINE_HISTORY");
             SendTimeLineHistory((TimeLineHistoryCmd *) cmd_node);
             break;
@@ -1660,6 +1666,7 @@ exec_replication_command(const char *cmd_string)
                 DestReceiver *dest = CreateDestReceiver(DestRemoteSimple);
                 VariableShowStmt *n = (VariableShowStmt *) cmd_node;

+                set_ps_display("SHOW");
                 /* syscache access needs a transaction environment */
                 StartTransactionCommand();
                 GetPGVariable(n->name, dest);
@@ -1680,8 +1687,11 @@ exec_replication_command(const char *cmd_string)
     SetQueryCompletion(&qc, CMDTAG_SELECT, 0);
     EndCommand(&qc, DestRemote, true);

-    /* Report to pgstat that this process is now idle */
-    pgstat_report_activity(STATE_IDLE, NULL);
+    /*
+     * We need not update ps display or pg_stat_activity, because PostgresMain
+     * will reset those to "idle".  But we must reset debug_query_string to
+     * ensure it doesn't become a dangling pointer.
+     */
     debug_query_string = NULL;

     return true;