pgsql: Integrate recovery.conf into postgresql.conf

On Sun, Nov 25, 2018 at 03:49:23PM +0000, Peter Eisentraut wrote:

Integrate recovery.conf into postgresql.conf

recovery.conf settings are now set in postgresql.conf (or other GUC
sources). Currently, all the affected settings are PGC_POSTMASTER;
this could be refined in the future case by case.

The buildfarm is unhappy after this one:
https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=culicidae&br=HEAD

If I use -DEXEC_BACKEND when compiling the tests complain about a
timeout in 003_recovery_targets. Here is what the buildfarm reports, I
can see the failure by myself as well:
# Postmaster PID for node "standby_6" is 26668
# poll_query_until timed out executing this query:
# SELECT '0/3022690'::pg_lsn <= pg_last_wal_replay_lsn()
# expecting this output:
# t
# last actual query output:
# f
# with stderr:
Timed out while waiting for standby to catch up at
t/003_recovery_targets.pl line 34.

Peter, could you look at that?
--
Michael

Hi

The buildfarm is unhappy after this one:
https://buildfarm.postgresql.org/cgi-bin/show_history.pl?nm=culicidae&amp;br=HEAD

If I use -DEXEC_BACKEND when compiling the tests complain about a
timeout in 003_recovery_targets. Here is what the buildfarm reports, I
can see the failure by myself as well:
# Postmaster PID for node "standby_6" is 26668
# poll_query_until timed out executing this query:
# SELECT '0/3022690'::pg_lsn <= pg_last_wal_replay_lsn()
# expecting this output:
# t
# last actual query output:
# f
# with stderr:
Timed out while waiting for standby to catch up at
t/003_recovery_targets.pl line 34.

I can reproduce this and notice wrong assign settings order. For example standby_6 has

recovery_target_name = '$recovery_name'
recovery_target_xid = '$recovery_txid'
recovery_target_time = '$recovery_time'

But recoveryTarget was set to RECOVERY_TARGET_XID
Without -DEXEC_BACKEND all fine.

As far as I understand the guc.c code with EXEC_BACKEND all processes uses different config processing logic. We serialize all GUC and restore at process start. And we sort GUC by name in build_guc_variables - so we restore settings in wrong order. I was afraid that the GUC system does not guarantee the order of settings...

What is preferable solution? Seems we can not simple change this logic.
I think i can reorganize all new recovery_target_* GUC into single one with format, for example, recovery_target = "xid:607" (was mentioned in patch discussion).

regards, Sergei

On 26/11/2018 13:32, Sergei Kornilov wrote:

Timed out while waiting for standby to catch up at
t/003_recovery_targets.pl line 34.

I can reproduce this and notice wrong assign settings order. For example standby_6 has

recovery_target_name = '$recovery_name'
recovery_target_xid = '$recovery_txid'
recovery_target_time = '$recovery_time'

But recoveryTarget was set to RECOVERY_TARGET_XID
Without -DEXEC_BACKEND all fine.

As far as I understand the guc.c code with EXEC_BACKEND all processes uses different config processing logic. We serialize all GUC and restore at process start. And we sort GUC by name in build_guc_variables - so we restore settings in wrong order. I was afraid that the GUC system does not guarantee the order of settings...

What is preferable solution? Seems we can not simple change this logic.

What is the reason for allowing multiple recovery_target_* settings and
taking the last one? Could we change this aspect to make this behave
better?

I think i can reorganize all new recovery_target_* GUC into single one with format, for example, recovery_target = "xid:607" (was mentioned in patch discussion).

That would be another option.

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

On Mon, Nov 26, 2018 at 02:06:36PM +0100, Peter Eisentraut wrote:

What is the reason for allowing multiple recovery_target_* settings and
taking the last one?

This came originally to check that recovery.conf handles correctly its
recovery targets when multiple ones are defined with the last one
winning, as users may want to append additional targets in an existing
recovery.conf. I have not checked the code in details, but I think that
we should be careful to keep that property, even with EXEC_BACKEND.
--
Michael

Hi

What is the reason for allowing multiple recovery_target_* settings and
taking the last one? Could we change this aspect to make this behave
better?

Correct response to user configuration, similar to old recovery.conf logic or other GUC - we allow redefine with rule "latest win".
I think we can disallow using multiple recovery_target_* settings. But currently i have no good idea how this check can be done in check_* callbacks.

regards, Sergei

Greetings,

* Michael Paquier (michael@paquier.xyz) wrote:

On Mon, Nov 26, 2018 at 02:06:36PM +0100, Peter Eisentraut wrote:

What is the reason for allowing multiple recovery_target_* settings and
taking the last one?

This came originally to check that recovery.conf handles correctly its
recovery targets when multiple ones are defined with the last one
winning, as users may want to append additional targets in an existing
recovery.conf. I have not checked the code in details, but I think that
we should be careful to keep that property, even with EXEC_BACKEND.

Ugh, no, I don't agree that this property makes sense to keep and since
we're making these changes, I'd argue that it's exactly the right time
to do away with that.

I would think we'd have the different GUCs and then the check functions
would only validate that they're valid inputs and then when we get to
the point where we're starting to do recovery we check and make sure
we've been given a sane overall configuration- which means that only
*one* is set, and it matches the recovery target requested.

As with backup, recovery should be extremely clear and straight-forward,
with alarms going off if there's something unclear or inconsistent. One
of the arguments around using postgresql.auto.conf (or any other managed
config file, as discussed elsewhere) is that it can be machine-processed
and therefore tooling around it can make sure that what goes into that
file is correct and sane and doesn't need to rely on being able to just
append things to the file and hope that it works.

Thanks!

Stephen

At 2018-11-26 10:18:52 -0500, sfrost@snowman.net wrote:

This came originally to check that recovery.conf handles correctly
its recovery targets when multiple ones are defined with the last
one winning […]

Ugh, no, I don't agree that this property makes sense to keep and
since we're making these changes, I'd argue that it's exactly the
right time to do away with that.

I strongly agree with everything Stephen said here.

-- Abhijit

On 11/26/18 10:35 AM, Abhijit Menon-Sen wrote:

At 2018-11-26 10:18:52 -0500, sfrost@snowman.net wrote:

This came originally to check that recovery.conf handles correctly
its recovery targets when multiple ones are defined with the last
one winning […]

Ugh, no, I don't agree that this property makes sense to keep and
since we're making these changes, I'd argue that it's exactly the
right time to do away with that.

I strongly agree with everything Stephen said here.

+1.

--
-David
david@pgmasters.net

Hello

I would think we'd have the different GUCs and then the check functions
would only validate that they're valid inputs and then when we get to
the point where we're starting to do recovery we check and make sure
we've been given a sane overall configuration- which means that only
*one* is set, and it matches the recovery target requested.

How about something like attached patch?

regards, Sergei

Greetings,

* Sergei Kornilov (sk@zsrv.org) wrote:

I would think we'd have the different GUCs and then the check functions
would only validate that they're valid inputs and then when we get to
the point where we're starting to do recovery we check and make sure
we've been given a sane overall configuration- which means that only
*one* is set, and it matches the recovery target requested.

How about something like attached patch?

I've not been following this very closely, but seems like
recovery_target_string is a bad idea.. Why not just make that
'recovery_target_immediate' and make it a boolean? Having a GUC that's
only got one valid value seems really odd.

+	if (targetSettingsCount > 1)
+		ereport(FATAL,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("can not specify multiple recovery targets")));

I think I would have done 'if (targetSettingsCount != 1)' here.

-# Multiple targets
-# Last entry has priority (note that an array respects the order of items
-# not hashes).

You could (and imv should) include a test that throws an expected error
if multiple targets are specified.

Thanks!

Stephen

On 2018-Nov-26, Stephen Frost wrote:

I would think we'd have the different GUCs and then the check functions
would only validate that they're valid inputs and then when we get to
the point where we're starting to do recovery we check and make sure
we've been given a sane overall configuration- which means that only
*one* is set, and it matches the recovery target requested.

I don't quite understand why it isn't sensible to specify more than one
and just stop recovery (or whatever) when at least one of them becomes
true. Maybe I want to terminate just before commit of transaction
12345, but no later than 2018-11-11 12:47 in any case.

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

On 2018-11-26 15:09:43 -0300, Alvaro Herrera wrote:

On 2018-Nov-26, Stephen Frost wrote:

I would think we'd have the different GUCs and then the check functions
would only validate that they're valid inputs and then when we get to
the point where we're starting to do recovery we check and make sure
we've been given a sane overall configuration- which means that only
*one* is set, and it matches the recovery target requested.

I don't quite understand why it isn't sensible to specify more than one
and just stop recovery (or whatever) when at least one of them becomes
true. Maybe I want to terminate just before commit of transaction
12345, but no later than 2018-11-11 12:47 in any case.

+1

Greetings,

On Mon, Nov 26, 2018 at 13:15 Andres Freund <andres@anarazel.de> wrote:

On 2018-11-26 15:09:43 -0300, Alvaro Herrera wrote:

On 2018-Nov-26, Stephen Frost wrote:

I would think we'd have the different GUCs and then the check functions
would only validate that they're valid inputs and then when we get to
the point where we're starting to do recovery we check and make sure
we've been given a sane overall configuration- which means that only
*one* is set, and it matches the recovery target requested.

I don't quite understand why it isn't sensible to specify more than one
and just stop recovery (or whatever) when at least one of them becomes
true. Maybe I want to terminate just before commit of transaction
12345, but no later than 2018-11-11 12:47 in any case.

I really have doubts about that being a serious use-case. If you know the
xid then you almost certainly want everything before that xid, and you use
the time stamp when you don’t know the xid (which is pretty much always..).

+1

Well, we could start with: that isn’t how things work today, nor how it
used to work before this patch, and we’ve not had anyone asking for it
except for people on this thread making things up.

Let’s at least fix the currently committed patch before adding new features
or changing how things in recovery work.

Thanks!

Stephen

On 2018-11-26 13:30:18 -0500, Stephen Frost wrote:

Well, we could start with: that isn’t how things work today, nor how it
used to work before this patch, and we’ve not had anyone asking for it
except for people on this thread making things up.

Thanks for assuming good faith.

Hello

I think I would have done 'if (targetSettingsCount != 1)' here.

Streaming replication does not have any recovery_target.
Well, i can check StandbyModeRequested too and allow 0 recovery_target only for standby mode.

 -# Multiple targets
 -# Last entry has priority (note that an array respects the order of items
 -# not hashes).

You could (and imv should) include a test that throws an expected error
if multiple targets are specified.

We have some example for checking unable-to-start DB? Did not find yet

Let’s at least fix the currently committed patch before adding new features or changing how things in recovery work.

+1

regards, Sergei

Greetings,

* Sergei Kornilov (sk@zsrv.org) wrote:

I think I would have done 'if (targetSettingsCount != 1)' here.

Streaming replication does not have any recovery_target.
Well, i can check StandbyModeRequested too and allow 0 recovery_target only for standby mode.

Ah. Agreed.

 -# Multiple targets
 -# Last entry has priority (note that an array respects the order of items
 -# not hashes).

You could (and imv should) include a test that throws an expected error
if multiple targets are specified.

We have some example for checking unable-to-start DB? Did not find yet

The TAP system in general supports expected failures, but that might
only be with the simple binaries, not sure. I know src/bin/pg_dump has
those though.

Let’s at least fix the currently committed patch before adding new features or changing how things in recovery work.

+1

Thanks!

Stephen

On 11/26/18 1:15 PM, Andres Freund wrote:

On 2018-11-26 15:09:43 -0300, Alvaro Herrera wrote:

On 2018-Nov-26, Stephen Frost wrote:

I would think we'd have the different GUCs and then the check functions
would only validate that they're valid inputs and then when we get to
the point where we're starting to do recovery we check and make sure
we've been given a sane overall configuration- which means that only
*one* is set, and it matches the recovery target requested.

I don't quite understand why it isn't sensible to specify more than one
and just stop recovery (or whatever) when at least one of them becomes
true. Maybe I want to terminate just before commit of transaction
12345, but no later than 2018-11-11 12:47 in any case.

+1

-1. At least for now.

Prior to this patch the last target specified in recovery.conf was the
one used, not a combination of them.

The committed patch did not propose to change that behavior as far as I
can see. Since there is no way to determine the order of GUCs like
there was for options in recovery.conf, I think it makes sense to
restrict it to a single target type for now. This is not exactly the
behavior we had before but I think it comes the closest.

Allowing multiple targets could be considered as a separate patch if
anyone is interested.

Regards,
--
-David
david@pgmasters.net

Hello

Updated patch attached:
- added testcase to verify database does not start with multiple recovery targets
- recovery_target = immediate was replaced with recovery_target_immediate bool GUC

thank you!

regards, Sergei

Greetings,

* Sergei Kornilov (sk@zsrv.org) wrote:

Updated patch attached:
- added testcase to verify database does not start with multiple recovery targets
- recovery_target = immediate was replaced with recovery_target_immediate bool GUC

I'd encourage you to look through the diff after you're finished hacking
before sending it to the list, in case things get left in that should be
removed, as below...

diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index c80b14e..cd29606 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -261,13 +261,21 @@ static bool restoredFromArchive = false;
static char *replay_image_masked = NULL;
static char *master_image_masked = NULL;

+/* recovery_target* original GUC values */
+char *recovery_target_string;

This shouldn't be here now, should it?

+char *recovery_target_xid_string;
+char *recovery_target_time_string;
+char *recovery_target_name_string;
+char *recovery_target_lsn_string;

/* options formerly taken from recovery.conf for archive recovery */

Shouldn't all of the above be listed under this comment..?

char	   *recoveryRestoreCommand = NULL;
char	   *recoveryEndCommand = NULL;
char	   *archiveCleanupCommand = NULL;
-RecoveryTargetType recoveryTarget = RECOVERY_TARGET_UNSET;
+static RecoveryTargetType recoveryTarget = RECOVERY_TARGET_UNSET;
bool		recoveryTargetInclusive = true;
int			recoveryTargetAction = RECOVERY_TARGET_ACTION_PAUSE;
+bool 		recoveryTargetImmediate;

Seems like this should, at least, be close to the char*'s that are
defining the other ways to specify a recovery targer.

TransactionId recoveryTargetXid;
TimestampTz recoveryTargetTime;
char *recoveryTargetName;
@@ -5381,9 +5389,42 @@ readRecoverySignalFile(void)
static void
validateRecoveryParameters(void)
{
+ uint8 targetSettingsCount = 0;
+
if (!ArchiveRecoveryRequested)
return;

+	/* Check for mutually exclusive target actions */
+	if (recoveryTargetImmediate)
+	{
+		++targetSettingsCount;
+		recoveryTarget = RECOVERY_TARGET_IMMEDIATE;
+	}
+	if (strcmp(recovery_target_name_string, "") != 0)
+	{
+		++targetSettingsCount;
+		recoveryTarget = RECOVERY_TARGET_NAME;
+	}
+	if (strcmp(recovery_target_lsn_string, "") != 0)
+	{
+		++targetSettingsCount;
+		recoveryTarget = RECOVERY_TARGET_LSN;
+	}
+	if (strcmp(recovery_target_xid_string, "") != 0)
+	{
+		++targetSettingsCount;
+		recoveryTarget = RECOVERY_TARGET_XID;
+	}
+	if (strcmp(recovery_target_time_string, "") != 0)
+	{
+		++targetSettingsCount;
+		recoveryTarget = RECOVERY_TARGET_TIME;
+	}
+	if (targetSettingsCount > 1)
+		ereport(FATAL,
+				(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+				 errmsg("can not specify multiple recovery targets")));

Doesn't look like you changed this based on my prior comment..?

Thanks!

Stephen