Continue work on changes to recovery.conf API

I completely forgot attach patch, sorry. Attached now

On 22 June 2018 at 02:48, Sergei Kornilov <sk@zsrv.org> wrote:

I completely forgot attach patch, sorry. Attached now

Sergei,

It's great to see you picking this up. I'll try to find time to review
this for the next CF cycle. Please poke me if you don't hear from me, I do
get distracted. It's long past time to get these changes in for good.

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

Hello

Commitfest 2018-09 is now open and, as planned, i create one entry: https://commitfest.postgresql.org/19/1711/
Also i attach new version due merge conflict with HEAD.

regards, Sergei

On 01/07/2018 13:45, Sergei Kornilov wrote:

Commitfest 2018-09 is now open and, as planned, i create one entry: https://commitfest.postgresql.org/19/1711/
Also i attach new version due merge conflict with HEAD.

Could you please describe in detail what this current patch does?

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello

Current patch moves recovery.conf settings into GUC system:
- if startup process found recovery.conf - it throw error
- recovery mode is turned on if new special file recovery.signal found
- standby_mode setting was removed. Standby mode can be enabled if startup found new special file standby.signal
- if present both standby.signal and recovery.signal - we use standby mode
(this design from previous thread)
- recovery parameters recovery_target_inclusive, recovery_target_timeline, recovery_target_action are new GUC without logic changes
- recovery_target (immediate), recovery_target_name, recovery_target_time, recovery_target_xid, recovery_target_lsn are replaced to recovery_target_type and recovery_target_value (was discussed and changed in previous thread)
- restore_command, archive_cleanup_command, recovery_end_command, primary_conninfo, primary_slot_name and recovery_min_apply_delay are just new GUC
- trigger_file was renamed to promote_signal_file for clarify (my change, in prev thread was removed)
- all new GUC are PGC_POSTMASTER (discussed in prev thread)
- pg_basebackup with -R (--write-recovery-conf) option will create standby.signal file and append primary_conninfo and primary_slot_name (if was used) to postgresql.auto.conf instead of writing new recovery.conf
- appropriate changes in tests and docs

regards, Sergei

Hello

Attached small update due the merge conflict with HEAD docs.

regards, Sergei

On 29/08/2018 17:43, Sergei Kornilov wrote:

Current patch moves recovery.conf settings into GUC system:
- if startup process found recovery.conf - it throw error

OK

- recovery mode is turned on if new special file recovery.signal found

OK

- standby_mode setting was removed. Standby mode can be enabled if startup found new special file standby.signal

OK

- if present both standby.signal and recovery.signal - we use standby mode
(this design from previous thread)

Didn't find the discussion on that and don't understand the reasons, but
seems OK and easy to change if we don't like it.

- recovery parameters recovery_target_inclusive, recovery_target_timeline, recovery_target_action are new GUC without logic changes

OK

- recovery_target (immediate), recovery_target_name, recovery_target_time, recovery_target_xid, recovery_target_lsn are replaced to recovery_target_type and recovery_target_value (was discussed and changed in previous thread)

I think this was the major point of contention. I reread the old
thread, and it's still not clear why we need to change this. _type and
_value look like an EAV system to me. GUC variables should be
verifiable independent of another variable. The other idea of using
only one variable seems better, but using two variables seems like a
poor compromise between one and five.

I propose to move this patch forward, keep the settings as they are. It
can be another patch to rename or reshuffle them.

- restore_command, archive_cleanup_command, recovery_end_command, primary_conninfo, primary_slot_name and recovery_min_apply_delay are just new GUC

OK

- trigger_file was renamed to promote_signal_file for clarify (my change, in prev thread was removed)

OK to add the "promote" prefix, but I'd prefer to keep the "trigger"
name. There is a lot of discussion and knowledge around that. Seems
unnecessary to change.

- all new GUC are PGC_POSTMASTER (discussed in prev thread)

OK

- pg_basebackup with -R (--write-recovery-conf) option will create standby.signal file and append primary_conninfo and primary_slot_name (if was used) to postgresql.auto.conf instead of writing new recovery.conf

I wonder if that would cause any problems. The settings in
postgresql.auto.conf are normally not PGC_POSTMASTER, otherwise you
couldn't use ALTER SYSTEM to put them there. Maybe it's OK.

- appropriate changes in tests and docs

looks good

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

I think this was the major point of contention. I reread the old
thread, and it's still not clear why we need to change this. _type and
_value look like an EAV system to me. GUC variables should be
verifiable independent of another variable.

No, they MUST be independently verifiable. The interactions between
the check_xxx functions in this patch are utterly unsafe. We've
learned that lesson before.

I propose to move this patch forward, keep the settings as they are. It
can be another patch to rename or reshuffle them.

Please do not commit this in this state.

I wonder if that would cause any problems. The settings in
postgresql.auto.conf are normally not PGC_POSTMASTER, otherwise you
couldn't use ALTER SYSTEM to put them there. Maybe it's OK.

Actually, that works fine. You do have to restart the postmaster
to make them take effect, but it's the same as if you edited the
main config file by hand.

regards, tom lane

Hi,

On 2018-09-28 16:36:35 -0400, Tom Lane wrote:

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:

I think this was the major point of contention. I reread the old
thread, and it's still not clear why we need to change this. _type and
_value look like an EAV system to me. GUC variables should be
verifiable independent of another variable.

No, they MUST be independently verifiable. The interactions between
the check_xxx functions in this patch are utterly unsafe. We've
learned that lesson before.

I'm not sure those concerns apply quite the same way here - we can move
the interdependent verification to the the point where they're used
first rather than relying on guc.c infrastructure. We already have
plenty of checks interdependent that way, without it causing many
problems. UI wise that's not too bad, if they're things that cannot be
changed arbitrarily at runtime.

Greetings,

Andres Freund

Andres Freund <andres@anarazel.de> writes:

On 2018-09-28 16:36:35 -0400, Tom Lane wrote:

No, they MUST be independently verifiable. The interactions between
the check_xxx functions in this patch are utterly unsafe. We've
learned that lesson before.

I'm not sure those concerns apply quite the same way here - we can move
the interdependent verification to the the point where they're used
first rather than relying on guc.c infrastructure.

And, if they're bad, what happens? Recovery fails?

I don't think it's a great idea to lose out on whatever error checking
the existing GUC infrastructure can provide, just so as to use a GUC
design that's not very nice in the first place.

regards, tom lane

Hello

thank you for review!

 - if present both standby.signal and recovery.signal - we use standby mode
 (this design from previous thread)

Didn't find the discussion on that and don't understand the reasons, but
seems OK and easy to change if we don't like it.

I meant this was implemented in previous proposed patch and i do not changed this. I didn't find the discussion on that too...

 - recovery_target (immediate), recovery_target_name, recovery_target_time, recovery_target_xid, recovery_target_lsn are replaced to recovery_target_type and recovery_target_value (was discussed and changed in previous thread)

I think this was the major point of contention. I reread the old
thread, and it's still not clear why we need to change this. _type and
_value look like an EAV system to me. GUC variables should be
verifiable independent of another variable. The other idea of using
only one variable seems better, but using two variables seems like a
poor compromise between one and five.

No, they MUST be independently verifiable. The interactions between the check_xxx functions in this patch are utterly unsafe.

Understood, thank you.
I will implement set of current five recovery_target* settings and would like to leave reorganization for futher pathes.

Not sure about one thing. All recovery_target settings change one internal recoveryTarget variable. GUC infrastructure will guarantee behavior if user erroneously set multiple different recovery_target*? I mean check_* and assign_* will be called in order and so latest config line wins?

- trigger_file was renamed to promote_signal_file for clarify (my change, in prev thread was removed)

OK to add the "promote" prefix, but I'd prefer to keep the "trigger"
name. There is a lot of discussion and knowledge around that. Seems
unnecessary to change.

Ok, will change to promote_trigger_file

 - pg_basebackup with -R (--write-recovery-conf) option will create standby.signal file and append primary_conninfo and primary_slot_name (if was used) to postgresql.auto.conf instead of writing new recovery.conf

I wonder if that would cause any problems. The settings in
postgresql.auto.conf are normally not PGC_POSTMASTER, otherwise you
couldn't use ALTER SYSTEM to put them there. Maybe it's OK.

Actually we can use ALTER SYSTEM to put PGC_POSTMASTER settings. I tried now "alter system set max_connections to 300;", postgresql.auto.conf was changed and affect max_connections during database start.
Of course, we can not reload PGC_POSTMASTER settings, but during start we call regular ParseConfigFile and can override PGC_POSTMASTER.

regards, Sergei

Hello

I complete new version of this patch. I've attached it.

In this version i remove proposed recovery_target_type/recovery_target_value and implement set of current settings: recovery_target (immediate), recovery_target_name, recovery_target_time, recovery_target_xid, recovery_target_lsn

  - trigger_file was renamed to promote_signal_file for clarify (my change, in prev thread was removed)

 OK to add the "promote" prefix, but I'd prefer to keep the "trigger"
 name. There is a lot of discussion and knowledge around that. Seems
 unnecessary to change.

Ok, will change to promote_trigger_file

Renamed to promote_trigger_file

Possible we need something like separate xlog_guc.c and header for related functions and definition?

regards, Sergei

Hi

I attached new version of this patch due merge conflict with pg_promote function.

regards, Sergei

On Fri, Sep 28, 2018 at 4:20 PM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:

- recovery_target (immediate), recovery_target_name, recovery_target_time, recovery_target_xid, recovery_target_lsn are replaced to recovery_target_type and recovery_target_value (was discussed and changed in previous thread)

I think this was the major point of contention. I reread the old
thread, and it's still not clear why we need to change this. _type and
_value look like an EAV system to me. GUC variables should be
verifiable independent of another variable. The other idea of using
only one variable seems better, but using two variables seems like a
poor compromise between one and five.

+1. I like one best, I can live with five, and I think two stinks.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 30/10/2018 14:30, Sergei Kornilov wrote:

I attached new version of this patch due merge conflict with pg_promote function.

This patch looks pretty good to me functionality-wise. There are some
code details to work through, listed below.

In this review, I'm skipping over your documentation changes. It seems
you have found all the places that mention recovery.conf and updated
them adequately. But I think in some cases we will need to take a few
steps back and reword a paragraph or section altogether. For example,
there will no longer be a reason for recovery-config.sgml to be a
separate chapter if it's all part of the main GUC system. We can work
through that later.

Code discussion:

- useless whitespace change in xlog.c

- I think we can drop logRecoveryParameters(). Users can now inspect
those parameters using the normal GUC mechanisms. (They were all DEBUG2
anyway, so it's not like many users will be missing this.) Then you can
also drop RecoveryTargetActionText().

- Why introduce MAXRESTOREPOINTNAMELEN? If you think this is useful,
then we could do it as a separate patch.

- Make the help text change in pg_archivecleanup.c similar to pg_standby.c.

- In pg_basebackup.c, duplicate defines PG_AUTOCONF_FILENAME and
STANDBY_SIGNAL_FILE. See that you can put those into a header file
somewhere.

- This stuff breaks translation strings:

    printf(_("  -R, --write-recovery-conf\n"
-            "                         write recovery.conf for
replication\n"));
+            "                         append replication config to "
PG_AUTOCONF_FILENAME "\n"
+            "                         and place " STANDBY_SIGNAL_FILE "
file\n"));

Use %s placeholders, or better yet replace it in a more compact way.

- The test function $node_standby->request_standby_mode() could use a
better name. How about set_ instead of request_ ?

- New string GUCs should have "" instead of NULL as their default in
guc.c. (Not sure whether that is strictly necessary, but it seems good
to be consistent.)

- Help strings in guc.c should end with a period.

- Renaming the promote and fallback_promote files seems unnecessary for
this patch, and I would take it out. Otherwise, the change in pg_ctl.c
is out of date with the comment above it.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Hello

Thank you! Here is patch update addressing your comments.

- useless whitespace change in xlog.c

Oops, did not notice. Fixed.

- I think we can drop logRecoveryParameters(). Users can now inspect
those parameters using the normal GUC mechanisms. (They were all DEBUG2
anyway, so it's not like many users will be missing this.) Then you can
also drop RecoveryTargetActionText().

Agreed, done.

- Why introduce MAXRESTOREPOINTNAMELEN? If you think this is useful,
then we could do it as a separate patch.

Reverted back. This was changed in prev proposal.

- Make the help text change in pg_archivecleanup.c similar to pg_standby.c.

Changed.

- In pg_basebackup.c, duplicate defines PG_AUTOCONF_FILENAME and
STANDBY_SIGNAL_FILE. See that you can put those into a header file
somewhere.

I move constants from xlog.h to xlog_internal.h. Also i revert back RECOVERY_COMMAND_FILE and RECOVERY_COMMAND_DONE into xlog.c (was moved to xlog.h few weeks ago)
But i have no good idea for PG_AUTOCONF_FILENAME. Seems most src/bin/ application uses hardcoded file path. How about miscadmin.h?

- This stuff breaks translation strings:

    printf(_(" -R, --write-recovery-conf\n"
- " write recovery.conf for
replication\n"));
+ " append replication config to "
PG_AUTOCONF_FILENAME "\n"
+ " and place " STANDBY_SIGNAL_FILE "
file\n"));

Use %s placeholders, or better yet replace it in a more compact way.

Maybe leave just "write configuration for replication" with explanation in docs, as was before?

- The test function $node_standby->request_standby_mode() could use a
better name. How about set_ instead of request_ ?

Sounds good, changed.

- New string GUCs should have "" instead of NULL as their default in
guc.c. (Not sure whether that is strictly necessary, but it seems good
to be consistent.)
- Help strings in guc.c should end with a period.

Fixed

- Renaming the promote and fallback_promote files seems unnecessary for
this patch, and I would take it out. Otherwise, the change in pg_ctl.c
is out of date with the comment above it.

Agreed, revert back.

regards, Sergei

On 13/11/2018 15:57, Sergei Kornilov wrote:

Thank you! Here is patch update addressing your comments.

I went over the patch and did a bunch of small refinements. I have also
updated the documentation more extensively. The attached patch is
committable to me.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Nov 20, 2018 at 12:07:12PM +0100, Peter Eisentraut wrote:

I went over the patch and did a bunch of small refinements. I have also
updated the documentation more extensively. The attached patch is
committable to me.

Thanks for putting energy into that.

Recovery is now initiated by a file recovery.signal. Standby mode is
initiated by a file standby.signal. The standby_mode setting is
gone. If a recovery.conf file is found, an error is issued.

I am having second thoughts about this part of the patch actually.
What's bad in keeping standby_mode and just rely on recovery.signal to
enforce recovery to happen? When the startup process starts all the
parameters should be loaded. That would also need less work from users
to switch to the new APIs. I think that there could be a point to
*merge* hot_standby and standby_mode actually into an enum, so keeping
standby_mode would help with that (not this patch work of course). The
barrier between recovery.trigger standby.trigger is also rather thin.

The trigger_file setting has been renamed to promote_trigger_file as
part of the move.

This rename looks fine.

pg_basebackup -R now appends settings to postgresql.auto.conf and
creates a standby.signal file.

Author: Simon Riggs <simon@2ndquadrant.com>
Author: Abhijit Menon-Sen <ams@2ndquadrant.com>
Author: Sergei Kornilov <sk@zsrv.org>

I think that Fujii Masao should also be listed as an author, or at least
get a mention. He was one of the first to work on this stuff.

Using separate GUCs for each target is fine by me. I would have thought
that 003_recovery_targets.pl needed some tweaks, so I am happy to see
that it is not the case.

So overall this stuff looks fine per its shape, just the part for
standby.signal may not justify breaking compatibility more than
necessary... The first mention of this part comes from
/messages/by-id/CANP8+jLoYSDjB5ip7wynVcckoE4OynHabUnB8pQMgBJgFKQpiw@mail.gmail.com
as far as I know.
--
Michael

On 21/11/2018 07:00, Michael Paquier wrote:

What's bad in keeping standby_mode and just rely on recovery.signal to
enforce recovery to happen? When the startup process starts all the
parameters should be loaded. That would also need less work from users
to switch to the new APIs. I think that there could be a point to
*merge* hot_standby and standby_mode actually into an enum, so keeping
standby_mode would help with that (not this patch work of course). The
barrier between recovery.trigger standby.trigger is also rather thin.

This wasn't my idea, so this is just my interpretation. The scenario
I'm wondering about is: You have a standby. So (under your system) you
set standby_mode=on and create recovery.trigger. Then you promote that
standby, so recovery.trigger is removed, but standby_mode=on stays.
Much time passes. At some point you want to do a PITR on that instance.
So you create a recovery.trigger, set some recovery parameters. But
you didn't notice that standby_mode=on is still set from way back when
-- and you create a mess.

One way to think about it is: Being a standby is a state, not a
configuration setting.

Btw., I'm not in love with the *.signal naming. I originally argued
against naming them *.trigger, but I don't like the alternative either.
But that's easy to change if someone has a better idea.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services