pgsql: Make CheckRequiredParameterValues() depend upon correct

sriggs@postgresql.org (Simon Riggs) writes:

Log Message:
-----------
Make CheckRequiredParameterValues() depend upon correct combination
of parameters. Fix bug report by Robert Haas that error message and
hint was incorrect if wrong mode parameters specified on master.
Internal changes only. Proposals for parameter simplification on
master/primary still under way.

Modified Files:
--------------
pgsql/src/backend/access/transam:
xlog.c (r1.401 -> r1.402)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/transam/xlog.c?r1=1.401&r2=1.402)
pgsql/src/include/catalog:
pg_control.h (r1.51 -> r1.52)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/catalog/pg_control.h?r1=1.51&r2=1.52)

This is a change in pg_control layout and requires a bump to the
pg_control version number (and hence forced initdb's all round).

I think it was quite premature to commit this when the design is
still under active discussion --- you may be forcing two rounds
of initdb on testers, when maybe only one or none would be enough.
Especially when you appear to be in the minority about what the design
should be.

regards, tom lane

On Fri, Apr 23, 2010 at 3:57 PM, Simon Riggs <sriggs@postgresql.org> wrote:

Log Message:
-----------
Make CheckRequiredParameterValues() depend upon correct combination
of parameters. Fix bug report by Robert Haas that error message and
hint was incorrect if wrong mode parameters specified on master.
Internal changes only. Proposals for parameter simplification on
master/primary still under way.

This is (1) premature, (2) not the consensus position, and unless I
mistaken, (3) wrong.

...Robert

On Fri, 2010-04-23 at 16:04 -0400, Tom Lane wrote:

sriggs@postgresql.org (Simon Riggs) writes:

Log Message:
-----------
Make CheckRequiredParameterValues() depend upon correct combination
of parameters. Fix bug report by Robert Haas that error message and
hint was incorrect if wrong mode parameters specified on master.
Internal changes only. Proposals for parameter simplification on
master/primary still under way.

Modified Files:
--------------
pgsql/src/backend/access/transam:
xlog.c (r1.401 -> r1.402)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/transam/xlog.c?r1=1.401&amp;r2=1.402)
pgsql/src/include/catalog:
pg_control.h (r1.51 -> r1.52)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/include/catalog/pg_control.h?r1=1.51&amp;r2=1.52)

This is a change in pg_control layout and requires a bump to the
pg_control version number (and hence forced initdb's all round).

OK

I think it was quite premature to commit this when the design is
still under active discussion --- you may be forcing two rounds
of initdb on testers, when maybe only one or none would be enough.
Especially when you appear to be in the minority about what the design
should be.

No intention of doing that. This change allows people to see what the
dependency actually is once the bug has been fixed. Change needs to
start from here, not from where we were before.

--
Simon Riggs www.2ndQuadrant.com

On Fri, 2010-04-23 at 16:05 -0400, Robert Haas wrote:

On Fri, Apr 23, 2010 at 3:57 PM, Simon Riggs <sriggs@postgresql.org> wrote:

Log Message:
-----------
Make CheckRequiredParameterValues() depend upon correct combination
of parameters. Fix bug report by Robert Haas that error message and
hint was incorrect if wrong mode parameters specified on master.
Internal changes only. Proposals for parameter simplification on
master/primary still under way.

This is (1) premature, (2) not the consensus position, and unless I
mistaken,

As I've said elsewhere it was intended to assist and clarify. Those
changes have nothing to do with the current discussion as I understood
it, which was about simplifying the user interface.

I will revoke.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon@2ndQuadrant.com> writes:

No intention of doing that. This change allows people to see what the
dependency actually is once the bug has been fixed. Change needs to
start from here, not from where we were before.

Well, actually, now that I've looked at the patch I think it's starting
from a fundamentally wrong position anyway. Checkpoint records are a
completely wrong mechanism for transmitting this data to slaves, because
a checkpoint is emitted *after* we do something, not *before* we do it.
In particular it's ludicrous to be looking at shutdown checkpoints to
try to determine whether the subsequent WAL will meet the slave's
requirements. There's no connection at all between what the GUC state
was at shutdown and what it might be after starting again.

A design that might work is
(1) store the active value of wal_mode in pg_control (but NOT as part of
the last-checkpoint-record image).
(2) invent a new WAL record type that is transmitted when we change
wal_mode.

Then, slaves could check whether the master's wal_mode is high enough
by looking at pg_control when they start plus any wal_mode_change
records they come across.

If we did this then we could get rid of those WAL record types that were
added to signify that information had been omitted from WAL at specific
times.

regards, tom lane

On Fri, Apr 23, 2010 at 4:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Simon Riggs <simon@2ndQuadrant.com> writes:

No intention of doing that. This change allows people to see what the
dependency actually is once the bug has been fixed. Change needs to
start from here, not from where we were before.

Well, actually, now that I've looked at the patch I think it's starting
from a fundamentally wrong position anyway.  Checkpoint records are a
completely wrong mechanism for transmitting this data to slaves, because
a checkpoint is emitted *after* we do something, not *before* we do it.
In particular it's ludicrous to be looking at shutdown checkpoints to
try to determine whether the subsequent WAL will meet the slave's
requirements.  There's no connection at all between what the GUC state
was at shutdown and what it might be after starting again.

A design that might work is
(1) store the active value of wal_mode in pg_control (but NOT as part of
the last-checkpoint-record image).
(2) invent a new WAL record type that is transmitted when we change
wal_mode.

Well, right now wal_mode would only be able to be changed at server
restart. Eventually we might relax that, but I think there are some
restrictions on how we can do it - like maybe needing to wait until
all the transactions running at the time the change was decided on
have committed, or, well, I'm not sure.

...Robert

On Fri, 2010-04-23 at 16:44 -0400, Tom Lane wrote:

There's no connection at all between what the GUC state
was at shutdown and what it might be after starting again.

A design that might work is
(1) store the active value of wal_mode in pg_control (but NOT as part of
the last-checkpoint-record image).
(2) invent a new WAL record type that is transmitted when we change
wal_mode.

Then, slaves could check whether the master's wal_mode is high enough
by looking at pg_control when they start plus any wal_mode_change
records they come across.

Seems OK on standby side. On the primary there are some other points,
mentioned on other thread as to when we can change wal_mode.

If we did this then we could get rid of those WAL record types that were
added to signify that information had been omitted from WAL at specific
times.

Please.

--
Simon Riggs www.2ndQuadrant.com

Robert Haas <robertmhaas@gmail.com> writes:

On Fri, Apr 23, 2010 at 4:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

A design that might work is
(1) store the active value of wal_mode in pg_control (but NOT as part of
the last-checkpoint-record image).
(2) invent a new WAL record type that is transmitted when we change
wal_mode.

Well, right now wal_mode would only be able to be changed at server
restart.

Right, but slave servers won't find out about the change until the first
checkpoint after the start. Which is Too Late.

regards, tom lane

On Fri, Apr 23, 2010 at 4:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Simon Riggs <simon@2ndQuadrant.com> writes:

No intention of doing that. This change allows people to see what the
dependency actually is once the bug has been fixed. Change needs to
start from here, not from where we were before.

Well, actually, now that I've looked at the patch I think it's starting
from a fundamentally wrong position anyway.  Checkpoint records are a
completely wrong mechanism for transmitting this data to slaves, because
a checkpoint is emitted *after* we do something, not *before* we do it.
In particular it's ludicrous to be looking at shutdown checkpoints to
try to determine whether the subsequent WAL will meet the slave's
requirements.  There's no connection at all between what the GUC state
was at shutdown and what it might be after starting again.

A design that might work is
(1) store the active value of wal_mode in pg_control (but NOT as part of
the last-checkpoint-record image).
(2) invent a new WAL record type that is transmitted when we change
wal_mode.

Then, slaves could check whether the master's wal_mode is high enough
by looking at pg_control when they start plus any wal_mode_change
records they come across.

If we did this then we could get rid of those WAL record types that were
added to signify that information had been omitted from WAL at specific
times.

<dons project manager hat>

I notice that Heikki's patch doesn't include doing the above. Should
we? If so, who's going to do it?

...Robert

Robert Haas wrote:

On Fri, Apr 23, 2010 at 4:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Well, actually, now that I've looked at the patch I think it's starting
from a fundamentally wrong position anyway. Checkpoint records are a
completely wrong mechanism for transmitting this data to slaves, because
a checkpoint is emitted *after* we do something, not *before* we do it.
In particular it's ludicrous to be looking at shutdown checkpoints to
try to determine whether the subsequent WAL will meet the slave's
requirements. There's no connection at all between what the GUC state
was at shutdown and what it might be after starting again.

A design that might work is
(1) store the active value of wal_mode in pg_control (but NOT as part of
the last-checkpoint-record image).
(2) invent a new WAL record type that is transmitted when we change
wal_mode.

Then, slaves could check whether the master's wal_mode is high enough
by looking at pg_control when they start plus any wal_mode_change
records they come across.

If we did this then we could get rid of those WAL record types that were
added to signify that information had been omitted from WAL at specific
times.

<dons project manager hat>

I notice that Heikki's patch doesn't include doing the above. Should
we? If so, who's going to do it?

I'll give it a shot.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

Heikki Linnakangas wrote:

Robert Haas wrote:

On Fri, Apr 23, 2010 at 4:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Well, actually, now that I've looked at the patch I think it's starting
from a fundamentally wrong position anyway. Checkpoint records are a
completely wrong mechanism for transmitting this data to slaves, because
a checkpoint is emitted *after* we do something, not *before* we do it.
In particular it's ludicrous to be looking at shutdown checkpoints to
try to determine whether the subsequent WAL will meet the slave's
requirements. There's no connection at all between what the GUC state
was at shutdown and what it might be after starting again.

A design that might work is
(1) store the active value of wal_mode in pg_control (but NOT as part of
the last-checkpoint-record image).
(2) invent a new WAL record type that is transmitted when we change
wal_mode.

Then, slaves could check whether the master's wal_mode is high enough
by looking at pg_control when they start plus any wal_mode_change
records they come across.

If we did this then we could get rid of those WAL record types that were
added to signify that information had been omitted from WAL at specific
times.

<dons project manager hat>

I notice that Heikki's patch doesn't include doing the above. Should
we? If so, who's going to do it?

I'll give it a shot.

Ok, here's a patch that includes the changes to add new wal_mode GUC
(http://archives.postgresql.org/message-id/4BD581A6.60602@enterprisedb.com),
and implements Tom's design to keep a copy of wal_mode and the
max_connections, max_prepared_xacts and max_locks_per_xact settings in
pg_control.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Tue, Apr 27, 2010 at 5:09 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

Ok, here's a patch that includes the changes to add new wal_mode GUC
(http://archives.postgresql.org/message-id/4BD581A6.60602@enterprisedb.com),
and implements Tom's design to keep a copy of wal_mode and the
max_connections, max_prepared_xacts and max_locks_per_xact settings in
pg_control.

I have some comments:

config.sgml

<literal>on</literal>. It is thought that there is little
measurable difference in performance from using this feature, so
feedback is welcome if any production impacts are noticeable.
It is likely that this parameter will be removed in later releases.

Is this description still required for recovery_connections?

if (!XLogArchivingActive())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("WAL archiving is not active"),
errhint("archive_mode must be enabled at server start.")));

You need to change the error messages which refer to archive_mode,
like the above.

+ /*
+  * For Hot Standby, the WAL must be generated with 'hot_standby' mode,
+  * and we must have at least as many backend slots as the primary.
+  */
+ if (InArchiveRecovery && XLogRequestRecoveryConnections)
+ {
+   if (ControlFile->wal_mode < WAL_MODE_HOT_STANDBY)
+       ereport(ERROR,
+              (errmsg("recovery connections cannot start because
wal_mode was not set to 'hot_standby' on the WAL source server")));

This seems to always prevent the server from doing an archive recovery
since wal_mode is expected to be WAL_MODE_ARCHIVE in that case.

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Fujii Masao wrote:

config.sgml

<literal>on</literal>. It is thought that there is little
measurable difference in performance from using this feature, so
feedback is welcome if any production impacts are noticeable.
It is likely that this parameter will be removed in later releases.

Is this description still required for recovery_connections?

Hmm, I guess it was referring to setting recovery_connections in the
master, I don't see us removing that option from the standby in the
future. recovery_connections in the master is being replaced with the
wal_mode setting, so I guess that's not required anymore.

if (!XLogArchivingActive())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("WAL archiving is not active"),
errhint("archive_mode must be enabled at server start.")));

You need to change the error messages which refer to archive_mode,
like the above.

Hmm, I think we should change not only the error message, but the logic
too. There's two related checks there:

if (!XLogArchivingActive())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("WAL archiving is not active"),
errhint("archive_mode must be enabled at server start.")));

if (!XLogArchiveCommandSet())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("WAL archiving is not active"),
errhint("archive_command must be defined before "
"online backups can be made safely.")));

You can use streaming replication too to transport the WAL generated
during the backup, so I think we should just check that wal_mode>='archive'.

+ /*
+  * For Hot Standby, the WAL must be generated with 'hot_standby' mode,
+  * and we must have at least as many backend slots as the primary.
+  */
+ if (InArchiveRecovery && XLogRequestRecoveryConnections)
+ {
+   if (ControlFile->wal_mode < WAL_MODE_HOT_STANDBY)
+       ereport(ERROR,
+              (errmsg("recovery connections cannot start because
wal_mode was not set to 'hot_standby' on the WAL source server")));

This seems to always prevent the server from doing an archive recovery
since wal_mode is expected to be WAL_MODE_ARCHIVE in that case.

No, it doesn't prevent archive recovery. It only prevents hot standby if
wal_mode was not 'hot_standby' in the master. I think you missed the "&&
XLogRequestRecoveryConnections" condition above.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Tue, Apr 27, 2010 at 6:49 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

Fujii Masao wrote:

config.sgml

<literal>on</literal>.  It is thought that there is little
measurable difference in performance from using this feature, so
feedback is welcome if any production impacts are noticeable.
It is likely that this parameter will be removed in later releases.

Is this description still required for recovery_connections?

Hmm, I guess it was referring to setting recovery_connections in the
master, I don't see us removing that option from the standby in the
future. recovery_connections in the master is being replaced with the
wal_mode setting, so I guess that's not required anymore.

Agreed.

if (!XLogArchivingActive())
  ereport(ERROR,
    (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
     errmsg("WAL archiving is not active"),
     errhint("archive_mode must be enabled at server start.")));

You need to change the error messages which refer to archive_mode,
like the above.

Hmm, I think we should change not only the error message, but the logic
too. There's two related checks there:

      if (!XLogArchivingActive())
              ereport(ERROR,
                              (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                               errmsg("WAL archiving is not active"),
                               errhint("archive_mode must be enabled at server start.")));

      if (!XLogArchiveCommandSet())
              ereport(ERROR,
                              (errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
                               errmsg("WAL archiving is not active"),
                               errhint("archive_command must be defined before "
                                               "online backups can be made safely.")));

You can use streaming replication too to transport the WAL generated
during the backup, so I think we should just check that wal_mode>='archive'.

It's OK in pg_start_backup(), but seems NG in pg_stop_backup() since
it waits until some WAL files have been archived by the archiver. No?

+ /*
+  * For Hot Standby, the WAL must be generated with 'hot_standby' mode,
+  * and we must have at least as many backend slots as the primary.
+  */
+ if (InArchiveRecovery && XLogRequestRecoveryConnections)
+ {
+   if (ControlFile->wal_mode < WAL_MODE_HOT_STANDBY)
+       ereport(ERROR,
+              (errmsg("recovery connections cannot start because
wal_mode was not set to 'hot_standby' on the WAL source server")));

This seems to always prevent the server from doing an archive recovery
since wal_mode is expected to be WAL_MODE_ARCHIVE in that case.

No, it doesn't prevent archive recovery. It only prevents hot standby if
wal_mode was not 'hot_standby' in the master. I think you missed the "&&
XLogRequestRecoveryConnections" condition above.

Even if we do only archive recovery, XLogRequestRecoveryConnections
might be TRUE. Or we need to ensure that the recovery_connection is
FALSE in the postgresql.conf before starting archive recovery?

And I tried archive recovery, and encountered the following error.

LOG: starting archive recovery
LOG: restored log file "000000010000000000000001" from archive
FATAL: recovery connections cannot start because wal_mode was not
set to 'hot_standby' on the WAL source server
LOG: startup process (PID 32512) exited with exit code 1
LOG: aborting startup due to startup process failure

XLOG-related parameters in postgresql.conf
archive_mode = on
archive_command = 'cp %p ../data.arh/%f'
wal_mode = archive
recovery_connections = on

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Fujii Masao wrote:

On Tue, Apr 27, 2010 at 6:49 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

Fujii Masao wrote:

if (!XLogArchivingActive())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("WAL archiving is not active"),
errhint("archive_mode must be enabled at server start.")));

You need to change the error messages which refer to archive_mode,
like the above.

Hmm, I think we should change not only the error message, but the logic
too. There's two related checks there:

if (!XLogArchivingActive())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("WAL archiving is not active"),
errhint("archive_mode must be enabled at server start.")));

if (!XLogArchiveCommandSet())
ereport(ERROR,
(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
errmsg("WAL archiving is not active"),
errhint("archive_command must be defined before "
"online backups can be made safely.")));

You can use streaming replication too to transport the WAL generated
during the backup, so I think we should just check that wal_mode>='archive'.

It's OK in pg_start_backup(), but seems NG in pg_stop_backup() since
it waits until some WAL files have been archived by the archiver. No?

Good point, that logic would need to be changed too. Should it simply
return immediately if archive_mode=off?

+ /*
+  * For Hot Standby, the WAL must be generated with 'hot_standby' mode,
+  * and we must have at least as many backend slots as the primary.
+  */
+ if (InArchiveRecovery && XLogRequestRecoveryConnections)
+ {
+   if (ControlFile->wal_mode < WAL_MODE_HOT_STANDBY)
+       ereport(ERROR,
+              (errmsg("recovery connections cannot start because
wal_mode was not set to 'hot_standby' on the WAL source server")));

This seems to always prevent the server from doing an archive recovery
since wal_mode is expected to be WAL_MODE_ARCHIVE in that case.

No, it doesn't prevent archive recovery. It only prevents hot standby if
wal_mode was not 'hot_standby' in the master. I think you missed the "&&
XLogRequestRecoveryConnections" condition above.

Even if we do only archive recovery, XLogRequestRecoveryConnections
might be TRUE. Or we need to ensure that the recovery_connection is
FALSE in the postgresql.conf before starting archive recovery?

Umm, yes, if you have recovery_connnections=on, it means you want hot
standby. And for that you need wal_mode='hot_standby'.

By "it doesn't prevent archive recovery" I meant "you can do traditional
archive recovery without hot standby". It doesn't matter how the WAL is
transported, via the archive or via streaming replication.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Tue, Apr 27, 2010 at 7:50 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

It's OK in pg_start_backup(), but seems NG in pg_stop_backup() since
it waits until some WAL files have been archived by the archiver. No?

Good point, that logic would need to be changed too. Should it simply
return immediately if archive_mode=off?

What if we wrongly set archive_mode to on and wal_mode to minimal?
I think that checking XLogArchivingActive() in pg_stop_backup() is
adequate.

+ /*
+  * For Hot Standby, the WAL must be generated with 'hot_standby' mode,
+  * and we must have at least as many backend slots as the primary.
+  */
+ if (InArchiveRecovery && XLogRequestRecoveryConnections)
+ {
+   if (ControlFile->wal_mode < WAL_MODE_HOT_STANDBY)
+       ereport(ERROR,
+              (errmsg("recovery connections cannot start because
wal_mode was not set to 'hot_standby' on the WAL source server")));

This seems to always prevent the server from doing an archive recovery
since wal_mode is expected to be WAL_MODE_ARCHIVE in that case.

No, it doesn't prevent archive recovery. It only prevents hot standby if
wal_mode was not 'hot_standby' in the master. I think you missed the "&&
XLogRequestRecoveryConnections" condition above.

Even if we do only archive recovery, XLogRequestRecoveryConnections
might be TRUE. Or we need to ensure that the recovery_connection is
FALSE in the postgresql.conf before starting archive recovery?

Umm, yes, if you have recovery_connnections=on, it means you want hot
standby. And for that you need wal_mode='hot_standby'.

Since the default value of recovery_connections is TRUE, I think that
the trouble which I encountered would often happen. We should disable
recovery_connections by default? Furthermore should move it from
postgresql.conf to recovery.conf?

On the other hand, I feel that recovery_connections=on in an archive
recovery is valid configuration *until* any read only connections are
requested. How about moving the above check to postmaster or backend?

Regards,

--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Tue, Apr 27, 2010 at 7:24 AM, Fujii Masao <masao.fujii@gmail.com> wrote:

On Tue, Apr 27, 2010 at 7:50 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:

It's OK in pg_start_backup(), but seems NG in pg_stop_backup() since
it waits until some WAL files have been archived by the archiver. No?

Good point, that logic would need to be changed too. Should it simply
return immediately if archive_mode=off?

What if we wrongly set archive_mode to on and wal_mode to minimal?
I think that checking XLogArchivingActive() in pg_stop_backup() is
adequate.

That case should be rejected at primary startup.

+ /*
+  * For Hot Standby, the WAL must be generated with 'hot_standby' mode,
+  * and we must have at least as many backend slots as the primary.
+  */
+ if (InArchiveRecovery && XLogRequestRecoveryConnections)
+ {
+   if (ControlFile->wal_mode < WAL_MODE_HOT_STANDBY)
+       ereport(ERROR,
+              (errmsg("recovery connections cannot start because
wal_mode was not set to 'hot_standby' on the WAL source server")));

This seems to always prevent the server from doing an archive recovery
since wal_mode is expected to be WAL_MODE_ARCHIVE in that case.

No, it doesn't prevent archive recovery. It only prevents hot standby if
wal_mode was not 'hot_standby' in the master. I think you missed the "&&
XLogRequestRecoveryConnections" condition above.

Even if we do only archive recovery, XLogRequestRecoveryConnections
might be TRUE. Or we need to ensure that the recovery_connection is
FALSE in the postgresql.conf before starting archive recovery?

Umm, yes, if you have recovery_connnections=on, it means you want hot
standby. And for that you need wal_mode='hot_standby'.

Since the default value of recovery_connections is TRUE, I think that
the trouble which I encountered would often happen. We should disable
recovery_connections by default? Furthermore should move it from
postgresql.conf to recovery.conf?

On the other hand, I feel that recovery_connections=on in an archive
recovery is valid configuration *until* any read only connections are
requested. How about moving the above check to postmaster or backend?

Or just not starting recovery connections, but still doing archive
recovery? I think in this case a WARNING might be adequate.

...Robert

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

Ok, here's a patch that includes the changes to add new wal_mode GUC
(http://archives.postgresql.org/message-id/4BD581A6.60602@enterprisedb.com),

I haven't read this in any detail, but why does it add inclusion of
pg_control.h to xlog.h? I don't see any reason for that in the actual
changes in xlog.h.

regards, tom lane

Tom Lane wrote:

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

Ok, here's a patch that includes the changes to add new wal_mode GUC
(http://archives.postgresql.org/message-id/4BD581A6.60602@enterprisedb.com),

I haven't read this in any detail, but why does it add inclusion of
pg_control.h to xlog.h? I don't see any reason for that in the actual
changes in xlog.h.

I put the enum for wal_mode to pg_control.h, so that it's available to
pg_controlinfo.c without #including xlog.h there. The
XLogArchivingActive() macro in xlog.h needs the enum values:

#define XLogArchivingActive() (XLogArchiveMode && wal_mode >=
WAL_MODE_ARCHIVE

I'm all ears for better suggestions, I didn't like that much either.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com