Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )
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
Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )
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 signature.asc Description: PGP signature
Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )
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 signature.asc Description: PGP signature
Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )
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 Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From e258d242fc0ef653aa7654af6fb065c7133ba430 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
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
Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )
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) ||
Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )
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 signature.asc Description: PGP signature
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )
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 signature.asc Description: PGP signature
Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )
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
signature.asc
Description: PGP signature
Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )
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
Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )
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
[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 pg_basebackup-generate-recovery-conf.v1.patch Description: Binary data
