[PATCH] minor bugfix for pg_basebackup (9.6 ~ )
Hi
In pg_basebackup's GenerateRecoveryConf() function, the value for
"primary_slot_name" is escaped, but the original, non-escaped value
is written. See attached patch.
This has been present since the code was added in 9.6 (commit 0dc848b0314).
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
pg_basebackup-generate-recovery-conf.v1.patchapplication/octet-stream; name=pg_basebackup-generate-recovery-conf.v1.patchDownload
diff --git a/src/bin/pg_basebackup/pg_basebackup.c b/src/bin/pg_basebackup/pg_basebackup.c
new file mode 100644
index 15f43f9..092e358
*** a/src/bin/pg_basebackup/pg_basebackup.c
--- b/src/bin/pg_basebackup/pg_basebackup.c
*************** GenerateRecoveryConf(PGconn *conn)
*** 1713,1719 ****
if (replication_slot)
{
escaped = escape_quotes(replication_slot);
! appendPQExpBuffer(recoveryconfcontents, "primary_slot_name = '%s'\n", replication_slot);
free(escaped);
}
--- 1713,1719 ----
if (replication_slot)
{
escaped = escape_quotes(replication_slot);
! appendPQExpBuffer(recoveryconfcontents, "primary_slot_name = '%s'\n", escaped);
free(escaped);
}
Hi
Oh. Replication slot name currently can contains only a-z0-9_ characters. So we can not actually write such recovery.conf, pg_basebackup will stop before. But perform escape_quotes on string and not use result - error anyway.
regards, Sergei
On 7/19/19 7:45 PM, Sergei Kornilov wrote:
Hi
Oh. Replication slot name currently can contains only a-z0-9_ characters. So
we can not actually write such recovery.conf, pg_basebackup will stop
before. But perform escape_quotes on string and not use result - error anyway.
Good point, it does actually fail with an error if an impossible slot name
is provided, so the escaping is superfluous anyway.
I'll take another look at it later as it's not exactly critical, just stuck
out when I was passing through the code.
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Fri, Jul 19, 2019 at 10:40:42PM +0900, Ian Barwick wrote:
Good point, it does actually fail with an error if an impossible slot name
is provided, so the escaping is superfluous anyway.
FWIW, ReplicationSlotValidateName() gives the reason behind that
restriction:
Slot names may consist out of [a-z0-9_]{1,NAMEDATALEN-1} which should allow
the name to be used as a directory name on every supported OS.
I'll take another look at it later as it's not exactly critical, just stuck
out when I was passing through the code.
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.
--
Michael
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?
--
Michael
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
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.
--
Michael
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
Attachments:
pg_basebackup-generate-recovery-conf.v2.patchtext/x-patch; name=pg_basebackup-generate-recovery-conf.v2.patchDownload
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) ||
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
Attachments:
v3-0001-Don-t-uselessly-escape-a-string-that-doesn-t-need.patchtext/x-diff; charset=us-asciiDownload
From e258d242fc0ef653aa7654af6fb065c7133ba430 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera <alvherre@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+w1JZ61p8YGPdWQ@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
On Wed, Jul 24, 2019 at 11:23:30AM -0400, Alvaro Herrera wrote:
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.
Looks fine to me. A nit: addition of braces for the if block. Even
if that a one-liner, there is a comment so I think that this makes the
code more readable.
--
Michael
On 2019-Jul-25, Michael Paquier wrote:
On Wed, Jul 24, 2019 at 11:23:30AM -0400, Alvaro Herrera wrote:
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.Looks fine to me. A nit: addition of braces for the if block. Even
if that a one-liner, there is a comment so I think that this makes the
code more readable.
Yeah, I had removed those on purpose, but that was probably inconsistent
with my own reviews of others' patches. I pushed it with them.
--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Jul 26, 2019 at 05:52:43PM -0400, Alvaro Herrera wrote:
Yeah, I had removed those on purpose, but that was probably inconsistent
with my own reviews of others' patches. I pushed it with them.
Thanks!
--
Michael
On 7/27/19 6:52 AM, Alvaro Herrera wrote:
On 2019-Jul-25, Michael Paquier wrote:
On Wed, Jul 24, 2019 at 11:23:30AM -0400, Alvaro Herrera wrote:
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.Looks fine to me. A nit: addition of braces for the if block. Even
if that a one-liner, there is a comment so I think that this makes the
code more readable.Yeah, I had removed those on purpose, but that was probably inconsistent
with my own reviews of others' patches. I pushed it with them.
Thanks
Regards
Ian Barwick
--
Ian Barwick https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services