Horton

Thread

Commits

  1. Don't uselessly escape a string that doesn't need escaping

  2. pg_basebackup: Add --slot option

  1. [PATCH] minor bugfix for pg_basebackup (9.6 ~ )

    Ian Barwick <ian.barwick@2ndquadrant.com> — 2019-07-19T08:08:39Z

    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

  2. Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )

    Sergei Kornilov <sk@zsrv.org> — 2019-07-19T10:45:05Z

    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

  3. Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )

    Ian Barwick <ian.barwick@2ndquadrant.com> — 2019-07-19T13:40:42Z

    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

  4. Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )

    Michael Paquier <michael@paquier.xyz> — 2019-07-20T01:04:19Z

    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

  5. Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )

    Michael Paquier <michael@paquier.xyz> — 2019-07-22T07:36:00Z

    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

  6. Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-07-22T16:58:40Z

    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

  7. Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )

    Michael Paquier <michael@paquier.xyz> — 2019-07-23T08:10:23Z

    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

  8. Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )

    Ian Barwick <ian.barwick@2ndquadrant.com> — 2019-07-24T04:12:33Z

    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

  9. Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-07-24T15:23:30Z

    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

  10. Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )

    Michael Paquier <michael@paquier.xyz> — 2019-07-24T23:53:02Z

    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

  11. Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2019-07-26T21:52:43Z

    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

  12. Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )

    Michael Paquier <michael@paquier.xyz> — 2019-07-29T01:51:52Z

    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

  13. Re: [PATCH] minor bugfix for pg_basebackup (9.6 ~ )

    Ian Barwick <ian.barwick@2ndquadrant.com> — 2019-08-01T02:37:21Z

    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