Date: Thu, 20 Feb 2025 10:29:56 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
Cc: 'Michael Paquier' <michael@paquier.xyz>,
	"'pgsql-hackers@lists.postgresql.org'" <pgsql-hackers@lists.postgresql.org>
Subject: Re: ReplicationSlotRelease() crashes when the instance is in the
 single user mode
Message-ID: <Z7cEJKB3qvgOOQLf@ip-10-97-1-34.eu-west-3.compute.internal>
References: <OSCPR01MB14966ED588A0328DAEBE8CB25F5FA2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <Z7QllLMq7UbyXHAD@paquier.xyz>
 <OSCPR01MB149669EF7D087A588773B7E04F5FA2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <Z7Q7vo-wWDwWw-tO@paquier.xyz>
 <OSCPR01MB14966559522B9699E1F769128F5FA2@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <OSCPR01MB14966F234CD3121F51810D5CCF5C52@OSCPR01MB14966.jpnprd01.prod.outlook.com>
 <Z7ZxC-xL7y_b6Yrs@paquier.xyz>
 <OSCPR01MB14966FCE16ECCF29AE0502AF2F5C42@OSCPR01MB14966.jpnprd01.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <OSCPR01MB14966FCE16ECCF29AE0502AF2F5C42@OSCPR01MB14966.jpnprd01.prod.outlook.com>
Archived-At: <https://www.postgresql.org/message-id/Z7cEJKB3qvgOOQLf%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

Hi,

On Thu, Feb 20, 2025 at 02:22:41AM +0000, Hayato Kuroda (Fujitsu) wrote:
> Dear Michael,
> 

Thanks for the report and the patch!

> > I did not check how these call behave individually, just a few
> > comments while putting my eyes on the patch.
> > 
> > +	if (!IsUnderPostmaster)
> > +		elog(ERROR,
> > +			 "slot operation is prohibited in the single user mode");
> > 
> > elog() should not be used for failures that can be user-facing as this
> > would not provide any translation.
> 
> I intentionally used elog() because I thought single user mode is not user-facing.
> But it is OK for me to use ereport() instead.

Yeah, I think it's also about using ereport for cases that we think can happen (
and so needs to be translated). In this case, it clearly can happen so ereport()
is better.

> > I'd suggest rewording the error message to provide some more context,
> > as well, say:
> > "cannot use %s in single-user mode", "function_name"
> 
> Fixed. PSA new version

=== 1

Nit:

"cannot use function %s in single-user mode", "function_name"" maybe? (that would
somehow match the existing ""user-defined types cannot use subscripting function %s")

=== 2

+ "pg_copy_replication_slot")));

s/pg_copy_replication_slot/copy_replication_slot/ maybe? As it is the function
that would report the error actually (could be called from
pg_copy_logical_replication_slot_[a|b|c] and pg_copy_physical_replication_slot_[a|b]
though).

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com