On 2019-Jul-24, Ian Barwick wrote: > It'd be better if such a hypothetical option validated the provided > slot name anwyay, to prevent later surprises.
Hmm, but what would we do if the validation failed? > Revised patch attached, which as Alvaro suggests removes the escaping > and adds a comment explaining why the raw value can be passed as-is. Heh, yesterday I revised the original patch as attached and was about to push when the bell rang. I like this one because it keeps the comment to one line and it mentions the function name in charge of the validation (useful for grepping later on). It's a bit laconic because of the long function name and the desire to keep it to one line, but it seems sufficient to me. BTW upper case letters are not allowed :-) -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From e258d242fc0ef653aa7654af6fb065c7133ba430 Mon Sep 17 00:00:00 2001 From: Alvaro Herrera <alvhe...@alvh.no-ip.org> Date: Tue, 23 Jul 2019 17:48:18 -0400 Subject: [PATCH v3] Don't uselessly escape a string that doesn't need escaping Per gripe from Ian Barwick Discussion: https://postgr.es/m/cabvvfjwnnnkb8chstlhktsvl1+g6bvcv+57+w1jz61p8ygp...@mail.gmail.com --- src/bin/pg_basebackup/pg_basebackup.c | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c index 77a7c148ba..e7755e807d 100644 --- a/src/bin/pg_basebackup/pg_basebackup.c +++ b/src/bin/pg_basebackup/pg_basebackup.c @@ -1715,11 +1715,9 @@ GenerateRecoveryConf(PGconn *conn) free(escaped); if (replication_slot) - { - escaped = escape_quotes(replication_slot); - appendPQExpBuffer(recoveryconfcontents, "primary_slot_name = '%s'\n", replication_slot); - free(escaped); - } + /* unescaped: ReplicationSlotValidateName allows [a-z0-9_] only */ + appendPQExpBuffer(recoveryconfcontents, "primary_slot_name = '%s'\n", + replication_slot); if (PQExpBufferBroken(recoveryconfcontents) || PQExpBufferDataBroken(conninfo_buf)) -- 2.17.1
