On 7/23/19 5:10 PM, Michael Paquier wrote:
On Mon, Jul 22, 2019 at 12:58:40PM -0400, Alvaro Herrera wrote:
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?
No objections with doing that either, really. Perhaps you would
prefer pushing a patch among those lines by yourself?
One argument that I got in mind to justify the escaping would be if we
add a new feature in pg_basebackup to write a new set of recovery
options on an existing data folder, which does not require an option.
In this case, if the escaping does not exist, starting the server
would fail with a confusing parsing error if a quote is added to the
slot name. But if the escaping is done, then we would get a correct
error that the replication slot value includes an incorrect character.
If such an hypothetical option is added, most likely this would be
noticed anyway, so that's mainly nannyism from my side.
It'd be better if such a hypothetical option validated the provided
slot name anwyay, to prevent later surprises.
Revised patch attached, which as Alvaro suggests removes the escaping
and adds a comment explaining why the raw value can be passed as-is.
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
new file mode 100644
index 77a7c14..9c910cb
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
*************** GenerateRecoveryConf(PGconn *conn)
*** 1716,1724 ****
if (replication_slot)
{
! escaped = escape_quotes(replication_slot);
appendPQExpBuffer(recoveryconfcontents, "primary_slot_name = '%s'\n", replication_slot);
- free(escaped);
}
if (PQExpBufferBroken(recoveryconfcontents) ||
--- 1716,1727 ----
if (replication_slot)
{
! /*
! * The slot name does not need escaping as it can only consist of [a-zA-Z0-9_].
! * The validity of the name has already been proven, as a slot must have been
! * successfully created with that name to reach this point.
! */
appendPQExpBuffer(recoveryconfcontents, "primary_slot_name = '%s'\n", replication_slot);
}
if (PQExpBufferBroken(recoveryconfcontents) ||
