Horton

Thread

Commits

  1. Fix bug in pg_basebackup -F plain -R.

  2. Split out recovery confing-writing code from pg_basebackup

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

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-02-10T07:58:56Z

    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

  2. Re: pg_basebackup -F plain -R overwrites postgresql.auto.conf

    Sergei Kornilov <sk@zsrv.org> — 2020-02-10T08:23:49Z

    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

  3. Re: pg_basebackup -F plain -R overwrites postgresql.auto.conf

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-02-10T08:41:40Z

    
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

  4. Re: pg_basebackup -F plain -R overwrites postgresql.auto.conf

    Magnus Hagander <magnus@hagander.net> — 2020-02-10T12:35:29Z

    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/

  5. Re: pg_basebackup -F plain -R overwrites postgresql.auto.conf

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-02-10T15:28:05Z

    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

  6. Re: pg_basebackup -F plain -R overwrites postgresql.auto.conf

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-02-10T15:28:43Z

    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

  7. Re: pg_basebackup -F plain -R overwrites postgresql.auto.conf

    Fujii Masao <masao.fujii@oss.nttdata.com> — 2020-02-12T00:14:37Z

    
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