Thread: [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

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

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

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