Re: Feature Request: pg_replication_master()

From: Simon Riggs <simon(at)2ndQuadrant(dot)com>
To: Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
Cc: Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>, PostgreSQL-development <pgsql-hackers(at)postgresql(dot)org>, Magnus Hagander <magnus(at)hagander(dot)net>
Subject: Re: Feature Request: pg_replication_master()
Date: 2012-12-24 15:13:59
Message-ID: CA+U5nMK8n=sQ-xPvBVtiCS3NbVObjUVM5xBR+FAeQN-RjjGxSQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 23 December 2012 15:24, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:

> The latest patch is the following. Of course, this cannot be applied
> cleanly in HEAD.
> http://archives.postgresql.org/message-id/CAHGQGwHB==cJehmE6JiuY-kNutRx-k3YWQziEg1gPNb3CK6N3A@mail.gmail.com
>
>> Just by looking at the last few posts, this seems like a no brainer. The
>> impression I get is that there's a patch that's otherwise ready to be
>> applied, but Simon and some others want to have backwards-compatiblity added
>> to it first.

The patch isn't ready to be applied. Reviewing this patch again from
scratch, it does the following...

1. Makes recovery.conf parameters into GUCs
2. Moves these parameters to postgresql.conf
3. Changes all the docs referring to recovery.conf

What the patch doesn't change is the requirement to have a file that
causes the server to place itself into archive recovery. So there is
no more recovery.conf and instead we have a file called
recovery.trigger instead.

We now support the concept of multiple .conf files, so continuing to
use a file called recovery.conf for those parameters works just fine
now without much effort. Note also that the recovery.conf requirement
to put things in quotes still works when they are GUCs, its just no
longer a requirement like it used to be. So previous files will work
just fine.

I don't see any point doing (2) or (3), especially since it makes it a
huge patch and especially since the trend is to break down large
config files into smaller pieces for use by Chef and Puppet.

Given the continuing need for a file to trigger recovery, changing the
name of the file from recovery.conf to recovery.trigger just breaks
backwards compatibility for absolutely no gain whatsoever (as long as
we do (1)).

I think it would be much easier to do just (1) in this release.

What we do want is the ability to move recovery.conf elsewhere (if we
choose), so that data directory is not writable by anything but
postgres server. If we do that, we'll have to make the config
directory writable by the server, whereas it wasn't before, so that
can cause problems also. That's the most useful additional change that
I see in this area, even better than (1).

(Note that you can't tell which server is the master purely from
reading primary_conninfo, since that you can't tell if you're a
cascading standby, or not. As discussed elsewhere, we could actually
do some more pushups to identify the current

From my perspective this patch and its design wasn't well thought
through and on balance I'm happy it wasn't committed earlier.

From all of the above, I think its worth doing this
* allowing recovery.conf to be in a different directory
* make recovery config parameters into GUCs
* no other changes to way things currently work

I don't think that represents enough change to keep people happy, but
I don't see anything else useful being suggested in this patch. Other
design thoughts welcome, but personally, I think rushing this design
through at this stage is likely to require us to change the design
again in later releases.

Productive comments welcome,

Merry Christmas

--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Charles Gomes 2012-12-24 15:43:04 Re: Writing Trigger Functions in C
Previous Message Amit Kapila 2012-12-24 14:49:30 Re: Re: patch submission: truncate trailing nulls from heap rows to reduce the size of the null bitmap [Review]