Date: Wed, 16 Sep 2020 19:14:22 -0300
From: Alvaro Herrera <alvherre@2ndquadrant.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Paquier <michael@paquier.xyz>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Gripes about walsender command processing
Message-ID: <20200916221422.GA24095@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <1633417.1600274669@sss.pgh.pa.us>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2020-Sep-16, Tom Lane wrote:

> This looks moderately reasonable to me.  However, with the process
> title reporting I want to add, we're going to end up with a switch
> that looks like
> 
>  		case T_IdentifySystemCmd:
> +			set_ps_display("IDENTIFY_SYSTEM");
>  			IdentifySystem();
> +			EndReplicationCommand("IDENTIFY_SYSTEM");
>  			break;
>  
>  		case T_BaseBackupCmd:
> +			set_ps_display("BASE_BACKUP");
>  			PreventInTransactionBlock(true, "BASE_BACKUP");
>  			SendBaseBackup((BaseBackupCmd *) cmd_node);
> +			EndReplicationCommand("BASE_BACKUP");
>  			break;
> 
> which is starting to look a bit repetitive and copy-pasteo-prone.

Agreed.

> I don't see an easy way to improve on it though.  The only obvious
> alternative would be to put another switch before the main one that
> just fills a "const char *cmdtag" variable, but that seems ugly.

The alternative of doing the assignment in each case of the same switch
does not look too terrible:

 		case T_IdentifySystemCmd:
+			cmdtag = "IDENTIFY_SYSTEM";
+			set_ps_display(cmdtag);
 			IdentifySystem();
+			EndReplicationCommand(cmdtag);
 			break;
 
 		case T_BaseBackupCmd:
+			cmdtag = "BASE_BACKUP";
+			set_ps_display(cmdtag);
 			PreventInTransactionBlock(true, cmdtag);
 			SendBaseBackup((BaseBackupCmd *) cmd_node);
+			EndReplicationCommand(cmdtag);
 			break;


-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services