Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )

On 2019-Jul-22, Michael Paquier wrote:

> On Sat, Jul 20, 2019 at 10:04:19AM +0900, Michael Paquier wrote:
> > This restriction is unlikely going to be removed, still I would rather
> > keep the escaped logic in pg_basebackup. This is the usual,
> > recommended coding pattern, and there is a risk that folks refer to
> > this code block for their own fancy stuff, spreading the problem. The
> > intention behind the code is to use an escaped name as well. For
> > those reasons your patch is fine by me.
>
> Attempting to use a slot with an unsupported set of characters will
> lead beforehand to a failure when trying to fetch the WAL segments
> with START_REPLICATION, meaning that this spot will never be reached
> and that there is no active bug, but for the sake of consistency I see
> no problems with applying the fix on HEAD. So, are there any
> objections with that?

Maybe it's just me, but it seems weird to try to forestall a problem
that cannot occur by definition. I would rather remove the escaping,
and add a one-line comment explaining why we don't do it?

if (replication_slot)
/* needn't escape because slot name must comprise [a-zA-Z0-9_] only */
appendPQExpBuffer(recoveryconfcontents, "primary_slot_name = '%s'\n",
replication_slot);

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