pg_basebackup -F plain -R overwrites postgresql.auto.conf

Started by Fujii Masaoabout 6 years ago7 messageshackers
Jump to latest
#1Fujii Masao
masao.fujii@gmail.com

Hi,

I found that pg_basebackup -F plain -R *overwrites* postgresql.auto.conf
taken from the primary server with new primary_conninfo setting,
while pg_basebackup -F tar -R just *appends* it into the file. I think that
this is a bug and pg_basebackup -F plain -R should *append* the setting.
Thought?

I attached the patch to fix the bug. This patch should be back-patch to
v12.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

Attachments:

basebackup_append_conninfo.patchtext/plain; charset=UTF-8; name=basebackup_append_conninfo.patch; x-mac-creator=0; x-mac-type=0Download+1-1
In reply to: Fujii Masao (#1)
Re: pg_basebackup -F plain -R overwrites postgresql.auto.conf

Hello

Seems bug was introduced in caba97a9d9f4d4fa2531985fd12d3cd823da06f3 - in HEAD only

In REL_12_STABLE we have:

bool is_recovery_guc_supported = true;

if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_RECOVERY_GUC)
is_recovery_guc_supported = false;

snprintf(filename, MAXPGPATH, "%s/%s", basedir,
is_recovery_guc_supported ? "postgresql.auto.conf" : "recovery.conf");

cf = fopen(filename, is_recovery_guc_supported ? "a" : "w");

It looks correct: append mode for postgresql.auto.conf

In HEAD version is_recovery_guc_supported variable was replaced to inversed use_recovery_conf without change fopen mode.

regards, Sergei

#3Fujii Masao
masao.fujii@gmail.com
In reply to: Sergei Kornilov (#2)
Re: pg_basebackup -F plain -R overwrites postgresql.auto.conf

On 2020/02/10 17:23, Sergei Kornilov wrote:

Hello

Seems bug was introduced in caba97a9d9f4d4fa2531985fd12d3cd823da06f3 - in HEAD only

In REL_12_STABLE we have:

bool is_recovery_guc_supported = true;

if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_RECOVERY_GUC)
is_recovery_guc_supported = false;

snprintf(filename, MAXPGPATH, "%s/%s", basedir,
is_recovery_guc_supported ? "postgresql.auto.conf" : "recovery.conf");

cf = fopen(filename, is_recovery_guc_supported ? "a" : "w");

It looks correct: append mode for postgresql.auto.conf

In HEAD version is_recovery_guc_supported variable was replaced to inversed use_recovery_conf without change fopen mode.

Yes! Thanks for pointing out that!
So the patch needs to be applied only in master.

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters

#4Magnus Hagander
magnus@hagander.net
In reply to: Fujii Masao (#3)
Re: pg_basebackup -F plain -R overwrites postgresql.auto.conf

On Mon, Feb 10, 2020 at 9:41 AM Fujii Masao <masao.fujii@oss.nttdata.com> wrote:

On 2020/02/10 17:23, Sergei Kornilov wrote:

Hello

Seems bug was introduced in caba97a9d9f4d4fa2531985fd12d3cd823da06f3 - in HEAD only

In REL_12_STABLE we have:

bool is_recovery_guc_supported = true;

if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_RECOVERY_GUC)
is_recovery_guc_supported = false;

snprintf(filename, MAXPGPATH, "%s/%s", basedir,
is_recovery_guc_supported ? "postgresql.auto.conf" : "recovery.conf");

cf = fopen(filename, is_recovery_guc_supported ? "a" : "w");

It looks correct: append mode for postgresql.auto.conf

In HEAD version is_recovery_guc_supported variable was replaced to inversed use_recovery_conf without change fopen mode.

Yes! Thanks for pointing out that!
So the patch needs to be applied only in master.

+1. We should absolutely not be overwriting the auto conf.

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/

#5Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Fujii Masao (#3)
Re: pg_basebackup -F plain -R overwrites postgresql.auto.conf

On 2020-Feb-10, Fujii Masao wrote:

On 2020/02/10 17:23, Sergei Kornilov wrote:

Hello

Seems bug was introduced in caba97a9d9f4d4fa2531985fd12d3cd823da06f3 - in HEAD only

In REL_12_STABLE we have:

bool is_recovery_guc_supported = true;

if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_RECOVERY_GUC)
is_recovery_guc_supported = false;

snprintf(filename, MAXPGPATH, "%s/%s", basedir,
is_recovery_guc_supported ? "postgresql.auto.conf" : "recovery.conf");

cf = fopen(filename, is_recovery_guc_supported ? "a" : "w");

It looks correct: append mode for postgresql.auto.conf

In HEAD version is_recovery_guc_supported variable was replaced to inversed use_recovery_conf without change fopen mode.

Yes! Thanks for pointing out that!
So the patch needs to be applied only in master.

Yikes, thanks. Pushing in a minute.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#6Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Alvaro Herrera (#5)
Re: pg_basebackup -F plain -R overwrites postgresql.auto.conf

On 2020-Feb-10, Alvaro Herrera wrote:

On 2020-Feb-10, Fujii Masao wrote:

Yes! Thanks for pointing out that!
So the patch needs to be applied only in master.

Yikes, thanks. Pushing in a minute.

Actually, if you want to push it, be my guest.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#7Fujii Masao
masao.fujii@gmail.com
In reply to: Alvaro Herrera (#6)
Re: pg_basebackup -F plain -R overwrites postgresql.auto.conf

On 2020/02/11 0:28, Alvaro Herrera wrote:

On 2020-Feb-10, Alvaro Herrera wrote:

On 2020-Feb-10, Fujii Masao wrote:

Yes! Thanks for pointing out that!
So the patch needs to be applied only in master.

Yikes, thanks. Pushing in a minute.

Actually, if you want to push it, be my guest.

Yeah, I pushed the patch. Thanks!

Regards,

--
Fujii Masao
NTT DATA CORPORATION
Advanced Platform Technology Group
Research and Development Headquarters