Обсуждение: v12 pg_basebackup fails against older servers (take two)

Hi,

When I run pg_basebackup in v12 against v11, standby server fails to connecto
primary with the following error:

2019-10-22 09:28:23.673 UTC [2375] FATAL:  could not connect to the primary
server: invalid connection option "gssencmode"

When I remove this from recovery.conf, it works fine. Looks like a bug to me
(we need to preserve backward compatibility). Comments?

Regards,
--
Devrim Gündüz
Open Source Solution Architect, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR

On Tue, Oct 22, 2019 at 12:32:53PM +0300, Devrim Gündüz wrote:
> When I run pg_basebackup in v12 against v11, standby server fails to connecto
> primary with the following error:
>
> 2019-10-22 09:28:23.673 UTC [2375] FATAL:  could not connect to the primary
> server: invalid connection option "gssencmode"
>
> When I remove this from recovery.conf, it works fine. Looks like a bug to me
> (we need to preserve backward compatibility). Comments?

You are referring to the connection string generated in
primary_conninfo here, right?  It would be nice to be more compatible
here.  This can be simply fixed by having an extra filter in
GenerateRecoveryConfig() (different file between HEAD and
REL_12_STABLE).  I also think that there is more.  On HEAD,
channel_binding gets added to the connection string generated which
would equally cause a failure with pg_basebackup from HEAD used for a
v12 or older server.
--
Michael

Hi,

On Tue, 2019-10-22 at 19:16 +0900, Michael Paquier wrote:
> You are referring to the connection string generated in
> primary_conninfo here, right?

Right.

> It would be nice to be more compatible here.  This can be simply fixed by
> having an extra filter in GenerateRecoveryConfig() (different file between
> HEAD and REL_12_STABLE).  I also think that there is more.  On HEAD,
> channel_binding gets added to the connection string generated which
> would equally cause a failure with pg_basebackup from HEAD used for a
> v12 or older server.

https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=beeb8e2e0717065296dc7b32daba2d66f0f931dd

had a similar approach in backwards compatibility, so I also agree on fixing
whatever breaks it.

Regards,
--
Devrim Gündüz
Open Source Solution Architect, Red Hat Certified Engineer
Twitter: @DevrimGunduz , @DevrimGunduzTR

Greetings,

* Devrim Gündüz (devrim@gunduz.org) wrote:
> On Tue, 2019-10-22 at 19:16 +0900, Michael Paquier wrote:
> > You are referring to the connection string generated in
> > primary_conninfo here, right?
>
> Right.

I'm awful suspicious that there's other similar cases beyond this
particular one...

> > It would be nice to be more compatible here.  This can be simply fixed by
> > having an extra filter in GenerateRecoveryConfig() (different file between
> > HEAD and REL_12_STABLE).  I also think that there is more.  On HEAD,
> > channel_binding gets added to the connection string generated which
> > would equally cause a failure with pg_basebackup from HEAD used for a
> > v12 or older server.
>
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=beeb8e2e0717065296dc7b32daba2d66f0f931dd
>
> had a similar approach in backwards compatibility, so I also agree on fixing
> whatever breaks it.

Yeah, we clearly do want newer versions of pg_basebackup to work with
older versions of PG and therefore we should address this.

Here's just a quick rough-up of a patch (it compiles, I haven't tried it
out more than that) that adds in a check to skip gssencmode on older
versions.  If it seems like a reasonable approach then I can test it out
and deal with back-patching it and such.

Thoughts?

Thanks,

Stephen

On Tue, Oct 22, 2019 at 09:06:03AM -0400, Stephen Frost wrote:
> Here's just a quick rough-up of a patch (it compiles, I haven't tried it
> out more than that) that adds in a check to skip gssencmode on older
> versions.  If it seems like a reasonable approach then I can test it out
> and deal with back-patching it and such.
>
> Thoughts?

Here is a thought.  We could tackle the problem at its source and
track in internalPQconninfoOption the minimum version supported by a
parameter.  This way, we could make sure that libpq routines similar
to PQconninfo() never return an option which is not compatible with a
a live connection, and we won't forget that if the problem shows up
again because creating a new parameter would require to add a new
version number.  There is an advantage here: internalPQconninfoOption
is an internal structure, so this should be back-patchable.
--
Michael

Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Tue, Oct 22, 2019 at 09:06:03AM -0400, Stephen Frost wrote:
> > Here's just a quick rough-up of a patch (it compiles, I haven't tried it
> > out more than that) that adds in a check to skip gssencmode on older
> > versions.  If it seems like a reasonable approach then I can test it out
> > and deal with back-patching it and such.
>
> Here is a thought.  We could tackle the problem at its source and
> track in internalPQconninfoOption the minimum version supported by a
> parameter.  This way, we could make sure that libpq routines similar
> to PQconninfo() never return an option which is not compatible with a
> a live connection, and we won't forget that if the problem shows up
> again because creating a new parameter would require to add a new
> version number.  There is an advantage here: internalPQconninfoOption
> is an internal structure, so this should be back-patchable.

Yeah..  Something along those lines definitely seems like it'd be better
as that would force anyone adding new options to explicitly say which
server version the option makes sense for.  Would it make sene to have a
minimum and a maximum (and a "currently live" or some such indicator, so
we aren't changing the max every release)?

The other thought I had was if we should, perhaps, be skipping settings
whose values haven't been changed from the default value.  Currently, we
end up with a bunch of stuff that, in my experience anyway, just ends up
being confusing to people, without any particular benefit, like
'sslcompression=0' when SSL wasn't used, or 'krbsrvname=postgres' when
Kerberos/GSSAPI wasn't used...

Thanks,

Stephen

On Tue, Oct 22, 2019 at 09:53:45AM -0400, Stephen Frost wrote:
> Yeah..  Something along those lines definitely seems like it'd be better
> as that would force anyone adding new options to explicitly say which
> server version the option makes sense for.  Would it make sense to have a
> minimum and a maximum (and a "currently live" or some such indicator, so
> we aren't changing the max every release)?

Yeah.  A maximum may help to handle properly the cycling of deprecated
options in connstrs, so I see your point.  Not sure that this
"currently-live" indicator is something to care about if we know
already the range of versions supported by a parameter and the
version of the backend for a live connection.  My take is that it
would be more consistent to have a PG_MAJORVERSION_NUM for this
purpose in pg_config.h as well (I honestly don't like much the
existing tweaks for the major version numbers like "PG_VERSION_NUM /
100" in pg_basebackup.c & co for example).  If we were to have a
maximum, couldn't there also be issues when it comes to link a binary
with a version of libpq which has been compiled with a version of
Postgres older than the version of the binary?  For example, imagine a
version of libpq compiled with v11, used to link to a pg_basebackup
from v12..  (@_@)

> The other thought I had was if we should, perhaps, be skipping settings
> whose values haven't been changed from the default value.  Currently, we
> end up with a bunch of stuff that, in my experience anyway, just ends up
> being confusing to people, without any particular benefit, like
> 'sslcompression=0' when SSL wasn't used, or 'krbsrvname=postgres' when
> Kerberos/GSSAPI wasn't used...

Couldn't this become a problem if we were to change the default for
some parameters?  There has been a lot of talks for example about how
bad sslmode's default it for one, even if nobody has actually pulled
the trigger to change it.
--
Michael

Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:
> On Tue, Oct 22, 2019 at 09:53:45AM -0400, Stephen Frost wrote:
> > Yeah..  Something along those lines definitely seems like it'd be better
> > as that would force anyone adding new options to explicitly say which
> > server version the option makes sense for.  Would it make sense to have a
> > minimum and a maximum (and a "currently live" or some such indicator, so
> > we aren't changing the max every release)?
>
> Yeah.  A maximum may help to handle properly the cycling of deprecated
> options in connstrs, so I see your point.  Not sure that this
> "currently-live" indicator is something to care about if we know
> already the range of versions supported by a parameter and the
> version of the backend for a live connection.  My take is that it
> would be more consistent to have a PG_MAJORVERSION_NUM for this
> purpose in pg_config.h as well (I honestly don't like much the
> existing tweaks for the major version numbers like "PG_VERSION_NUM /
> 100" in pg_basebackup.c & co for example).  If we were to have a
> maximum, couldn't there also be issues when it comes to link a binary
> with a version of libpq which has been compiled with a version of
> Postgres older than the version of the binary?  For example, imagine a
> version of libpq compiled with v11, used to link to a pg_basebackup
> from v12..  (@_@)

Erm, your last concern is exactly why I was saying we'd have a
'currently live' indicator- so that it wouldn't be an issue to have an
older library connecting from a new application to a newer database.

> > The other thought I had was if we should, perhaps, be skipping settings
> > whose values haven't been changed from the default value.  Currently, we
> > end up with a bunch of stuff that, in my experience anyway, just ends up
> > being confusing to people, without any particular benefit, like
> > 'sslcompression=0' when SSL wasn't used, or 'krbsrvname=postgres' when
> > Kerberos/GSSAPI wasn't used...
>
> Couldn't this become a problem if we were to change the default for
> some parameters?  There has been a lot of talks for example about how
> bad sslmode's default it for one, even if nobody has actually pulled
> the trigger to change it.

That really depends on if we think that users will expect the
new-default behavior to be used, or the old-default to be.  If the user
didn't set anything explicitly when they ran the command in the first
place, then it would seem like they intended and expected the defaults
to be used.  Perhaps that's an even better answer- just only put into
the recovery.conf file what the user actually set instead of a bunch of
other stuff...

Thanks,

Stephen