Thread

pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <sriggs@postgresql.org> — 2010-04-23T19:57:19Z

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)

Re: pgsql: Make CheckRequiredParameterValues() depend upon correct

Tom Lane <tgl@sss.pgh.pa.us> — 2010-04-23T20:04:15Z

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

Re: pgsql: Make CheckRequiredParameterValues() depend upon correct

Robert Haas <robertmhaas@gmail.com> — 2010-04-23T20:05:59Z

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

Re: pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-23T20:07:28Z

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&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).

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

Re: pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-23T20:22:00Z

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

Re: [HACKERS] Re: pgsql: Make CheckRequiredParameterValues() depend upon correct

Tom Lane <tgl@sss.pgh.pa.us> — 2010-04-23T20:44:41Z

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

Re: [HACKERS] Re: pgsql: Make CheckRequiredParameterValues() depend upon correct

Robert Haas <robertmhaas@gmail.com> — 2010-04-23T21:27:21Z

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

Re: [HACKERS] Re: pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-23T21:32:13Z

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

Re: [HACKERS] Re: pgsql: Make CheckRequiredParameterValues() depend upon correct

Tom Lane <tgl@sss.pgh.pa.us> — 2010-04-23T22:24:46Z

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Robert Haas <robertmhaas@gmail.com> — 2010-04-26T13:19:58Z

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-26T14:46:35Z

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-27T08:09:56Z

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Fujii Masao <masao.fujii@gmail.com> — 2010-04-27T09:34:10Z

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-27T09:49:06Z

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Fujii Masao <masao.fujii@gmail.com> — 2010-04-27T10:12:33Z

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-27T10:50:36Z

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Fujii Masao <masao.fujii@gmail.com> — 2010-04-27T11:24:30Z

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Robert Haas <robertmhaas@gmail.com> — 2010-04-27T11:56:51Z

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Tom Lane <tgl@sss.pgh.pa.us> — 2010-04-27T15:42:58Z

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-27T17:18:28Z

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Tom Lane <tgl@sss.pgh.pa.us> — 2010-04-27T18:16:18Z

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Tom Lane wrote:
>> 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:

Oh, I see.

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

How about putting the enum {} declaration in xlog.h, and making the
field in pg_control.h just be declared "int"?  I'm not sure declaring
it as enum is a great idea anyway, since that makes the on-disk
representation dependent on a compiler's whim as to how wide the
enum will be.

			regards, tom lane

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-27T19:00:04Z

Tom Lane wrote:
> How about putting the enum {} declaration in xlog.h, and making the
> field in pg_control.h just be declared "int"?

I tried that at first, but the problem was with pg_controldata.c. In
bin/. I wanted it to print wal_mode in human-readable format, so it
needed the values of the enum from somewhere. I tried to "#include
<access/xlog.h>" in pg_controlinfo.c, but got a bunch of errors.

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Alvaro Herrera <alvherre@commandprompt.com> — 2010-04-27T19:20:30Z

Heikki Linnakangas escribió:
> Tom Lane wrote:
> > How about putting the enum {} declaration in xlog.h, and making the
> > field in pg_control.h just be declared "int"?
> 
> I tried that at first, but the problem was with pg_controldata.c. In
> bin/. I wanted it to print wal_mode in human-readable format, so it
> needed the values of the enum from somewhere. I tried to "#include
> <access/xlog.h>" in pg_controlinfo.c, but got a bunch of errors.

Hmm, AFAICS the problem with controldata is that it uses postgres_fe.h
instead of postgres.h.  It's a bit of a stretch to use the latter, but
maybe that's a better solution?  After all, it *is* poking into the
backend internals.

I know I had to hack around pg_controldata some time ago (I don't recall
what for) and found that it could be cleaned up like this.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
PostgreSQL Replication, Consulting, Custom Development, 24x7 support

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Tom Lane <tgl@sss.pgh.pa.us> — 2010-04-27T23:00:01Z

Alvaro Herrera <alvherre@commandprompt.com> writes:
> Hmm, AFAICS the problem with controldata is that it uses postgres_fe.h
> instead of postgres.h.  It's a bit of a stretch to use the latter, but
> maybe that's a better solution?  After all, it *is* poking into the
> backend internals.

I seem to recall that Solaris had problems with that due to dtrace
support or something?  However, we are doing it in pg_resetxlog,
so I suppose it's ok for pg_controldata as well.

			regards, tom lane

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Alvaro Herrera <alvherre@commandprompt.com> — 2010-04-27T23:28:29Z

Tom Lane escribió:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
> > Hmm, AFAICS the problem with controldata is that it uses postgres_fe.h
> > instead of postgres.h.  It's a bit of a stretch to use the latter, but
> > maybe that's a better solution?  After all, it *is* poking into the
> > backend internals.
> 
> I seem to recall that Solaris had problems with that due to dtrace
> support or something?

Hmm, I wonder if you're referring to the fact that Zdenek wanted to
restructure the headers for something?  I don't know if this was because
of compiler issues or the binary migration tool he was working on.

> However, we are doing it in pg_resetxlog, so I suppose it's ok for
> pg_controldata as well.

I hadn't noticed that, but yes.

-- 
Alvaro Herrera                                http://www.CommandPrompt.com/
The PostgreSQL Company - Command Prompt, Inc.

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-28T07:43:06Z

Tom Lane wrote:
> Alvaro Herrera <alvherre@commandprompt.com> writes:
>> Hmm, AFAICS the problem with controldata is that it uses postgres_fe.h
>> instead of postgres.h.  It's a bit of a stretch to use the latter, but
>> maybe that's a better solution?  After all, it *is* poking into the
>> backend internals.
> 
> I seem to recall that Solaris had problems with that due to dtrace
> support or something?  However, we are doing it in pg_resetxlog,
> so I suppose it's ok for pg_controldata as well.

Ok, did that. Here's an updated patch:

* made incorrect combinations of wal_mode and
archive_mode/max_wal_senders throw an ERROR instead of WARNING

* renamed wal_mode to wal_level

* reworded the documentation changes a bit

This doesn't contain any changes to pg_start_backup() yet, that's a
separate issue and still under discussion.

At commit, should I bump catversion, or PG_CONTROL_VERSION, or both? The
patch replaces the unlogged-operation WAL record with a record
containing current parameter values, and it changes pg_control. I'm
guessing both.

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-28T08:00:04Z

On Wed, 2010-04-28 at 10:43 +0300, Heikki Linnakangas wrote:

> * renamed wal_mode to wal_level

I'm wondering whether this should be a list rather than an enum? If we
add something in the future that adds more info to WAL but doesn't fit
the one-dimensional model this implements then we could be in trouble.
Should this be

e.g. wal_xxxx = feature2, feature3
e.g. wal_xxxx = feature3
e.g. wal_xxxx = feature1

recognising that some features require other features, so as an example
feature2 requires and implies feature1.

The word "level" implies a one-dimensionality that "mode" did not and I
feel a little uncertain about that term.

Other words: attributes, features, contents, info, options.
Hmm, wal_options sounds OK.

Anyway, just throwing out some ideas to make sure we're doing the right
thing with this.

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-28T08:27:20Z

Simon Riggs wrote:
> On Wed, 2010-04-28 at 10:43 +0300, Heikki Linnakangas wrote:
> 
>> * renamed wal_mode to wal_level
> 
> I'm wondering whether this should be a list rather than an enum? If we
> add something in the future that adds more info to WAL but doesn't fit
> the one-dimensional model this implements then we could be in trouble.
> Should this be
> 
> e.g. wal_xxxx = feature2, feature3
> e.g. wal_xxxx = feature3
> e.g. wal_xxxx = feature1
> 
> recognising that some features require other features, so as an example
> feature2 requires and implies feature1.
> 
> The word "level" implies a one-dimensionality that "mode" did not and I
> feel a little uncertain about that term.

That's the same concern Tom had at
http://archives.postgresql.org/message-id/17411.1272291825@sss.pgh.pa.us.
If we want to make it a list, I think it would be something like:

wal_extra_info = ''		# "archive", or "archive, hot_standby"

That seems OK to me. Still, I'd like to go with "level", it's short and
descriptive of the current options, and it's not that big a deal to
rename it if we add more options in the future.

It's hard to picture what the future options might be like. Someone had
an idea years ago (you even?) to add more information like table names
or primary keys to the WAL records, to make it easier to scrape
information from the WAL for 3rd party replication solutions. Like
feeding a slony replica from the WAL. Or maybe we'd want to fold
full_page_writes to the new GUC.

If it's just one or two extra feature orthogonal to the
minimal-archive-hot_standby axis, it might still be best to keep the
wal_level name, and make it possible to tack extra options to it like a
list. Like "wal_level='archive, featureX'". "Level" would still be a
good name, that's the main dimension.

If the dependencies between the features get complicated, though, I
doubt we can correctly guess the best representation for them now anyway.

That's just my 2 cents, if others feel like "level" is painting us to
the corner, "wal_mode" with the same values is the second best option IMO.

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-28T09:22:12Z

On Wed, 2010-04-28 at 11:27 +0300, Heikki Linnakangas wrote:
> It's hard to picture what the future options might be like. Someone
> had
> an idea years ago (you even?) to add more information like table names
> or primary keys to the WAL records, to make it easier to scrape
> information from the WAL for 3rd party replication solutions. Like
> feeding a slony replica from the WAL. Or maybe we'd want to fold
> full_page_writes to the new GUC.

Yeh, lots of ideas for adding value to WAL. WAL can be considered a
transport solution with many potential uses and properties.

I haven't ever suggested adding something that would only be available
to 3rd party solutions, perhaps that information was garbled. Yes, I did
suggest that Slony might use a WAL transport in future, as an option.

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Fujii Masao <masao.fujii@gmail.com> — 2010-04-28T09:54:21Z

On Wed, Apr 28, 2010 at 4:43 PM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
> Ok, did that. Here's an updated patch:

> +        During recovery, specifies whether or not you can connect and run
> +        queries to enable <xref linkend="hot-standby">. The default value is
> +        <literal>on</literal>.
> +        This parameter can only be set at server start. It is ignored when
> +        not in standby mode.

In the patch, I was not able to find any change which ignores
recovery_connections when standby_mode is OFF.

And that change spoils the combination of pg_standby and HS?
We drop the support for it?


> * renamed wal_mode to wal_level

We should replace "mode" with "level" in the document of wal_level
parameter as follows?

@@ -1368,9 +1368,9 @@ SET ENABLE_SEQSCAN TO OFF;
         read-only queries on a standby server.
         This parameter can only be set at server start.
        </para>
        <para>
-        In <literal>minimal</> mode, WAL-logging of some bulk operations, like
+        In <literal>minimal</> level, WAL-logging of some bulk operations, like
         <command>CREATE INDEX</>, <command>CLUSTER</> and <command>COPY</> on
         a table that was created or truncated in the same transaction can be
         safely skipped, which can make those operations much faster (see
         <xref linkend="populate-pitr">). But minimal WAL does not contain
@@ -1379,15 +1379,15 @@ SET ENABLE_SEQSCAN TO OFF;
         WAL archiving (<xref linkend="guc-archive-mode">) and streaming
         replication.
        </para>
        <para>
-        In <literal>hot_standby</> mode, the same information is logged as
-        in <literal>archive</> mode, plus information needed to reconstruct
+        In <literal>hot_standby</> level, the same information is logged as
+        in <literal>archive</> level, plus information needed to reconstruct
         the status of running transactions from the WAL. To enable read-only
         queries on a standby server, <varname>wal_level</> must be set to
         <literal>hot_standby</> on the primary. It is thought that there is
         little measurable difference in performance from using
-        <literal>hot_standby</> mode over <literal>archive</>, so feedback
+        <literal>hot_standby</> level over <literal>archive</>, so feedback
         is welcome if any production impacts are noticeable.
        </para>
       </listitem>
      </varlistentry>

Regards,

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-28T10:06:48Z

On Wed, 2010-04-28 at 18:54 +0900, Fujii Masao wrote:
> On Wed, Apr 28, 2010 at 4:43 PM, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.com> wrote:
> > Ok, did that. Here's an updated patch:
> 
> > +        During recovery, specifies whether or not you can connect and run
> > +        queries to enable <xref linkend="hot-standby">. The default value is
> > +        <literal>on</literal>.
> > +        This parameter can only be set at server start. It is ignored when
> > +        not in standby mode.
> 
> In the patch, I was not able to find any change which ignores
> recovery_connections when standby_mode is OFF.
> 
> And that change spoils the combination of pg_standby and HS?
> We drop the support for it?

Yeh, I hope nothing is being disabled in that area. Many people hope and
expect HS to work with 9.0 without any changes to their setup.

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-28T10:32:28Z

Fujii Masao wrote:
> On Wed, Apr 28, 2010 at 4:43 PM, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.com> wrote:
>> Ok, did that. Here's an updated patch:
> 
>> +        During recovery, specifies whether or not you can connect and run
>> +        queries to enable <xref linkend="hot-standby">. The default value is
>> +        <literal>on</literal>.
>> +        This parameter can only be set at server start. It is ignored when
>> +        not in standby mode.
> 
> In the patch, I was not able to find any change which ignores
> recovery_connections when standby_mode is OFF.
> 
> And that change spoils the combination of pg_standby and HS?
> We drop the support for it?

That sentence is poorly worded, recovery_connections works in archive
recovery too (and with pg_standby). I'll rephrase that to "It only has
effect during archive recovery or standby mode."

>> * renamed wal_mode to wal_level
> 
> We should replace "mode" with "level" in the document of wal_level
> parameter as follows?
> 
> ...

Yep.

Thanks!

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Tom Lane <tgl@sss.pgh.pa.us> — 2010-04-28T13:38:42Z

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> At commit, should I bump catversion, or PG_CONTROL_VERSION, or both? The
> patch replaces the unlogged-operation WAL record with a record
> containing current parameter values, and it changes pg_control. I'm
> guessing both.

You *must* bump PG_CONTROL_VERSION because the content of pg_control
changed.  The correct way to reflect a change in WAL contents is to
bump the WAL page magic number.  I don't see any change here in either
system catalog contents or user table contents, so I see no need to
touch catversion.

			regards, tom lane

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Tom Lane <tgl@sss.pgh.pa.us> — 2010-04-28T14:19:23Z

Simon Riggs <simon@2ndQuadrant.com> writes:
> On Wed, 2010-04-28 at 10:43 +0300, Heikki Linnakangas wrote:
>> * renamed wal_mode to wal_level

> I'm wondering whether this should be a list rather than an enum? If we
> add something in the future that adds more info to WAL but doesn't fit
> the one-dimensional model this implements then we could be in trouble.
> Should this be
> e.g. wal_xxxx = feature2, feature3
> e.g. wal_xxxx = feature3
> e.g. wal_xxxx = feature1

I'm a bit suspicious of going in this direction, mainly because
DateStyle has been such a PITA over the years.  It's not always obvious
to users whether adding or removing an item in a list causes something
to turn on or off.

In any case, the project's expectations for forward compatibility of
postgresql.conf settings have always been very low.  I don't think we
should try to design wal_mode to solve future problems, just the ones
we are faced with right now.  If it gets changed to look completely
different in some future version, that's not a problem.

			regards, tom lane

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-28T14:59:58Z

On Wed, 2010-04-28 at 10:19 -0400, Tom Lane wrote:

> I'm a bit suspicious of going in this direction, mainly because
> DateStyle has been such a PITA over the years.

LOL. Man that's a pain. That's an unfair brush though!

-- 
 Simon Riggs           www.2ndQuadrant.com

WAL page magic number (was Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct)

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-28T15:07:45Z

Tom Lane wrote:
> The correct way to reflect a change in WAL contents is to
> bump the WAL page magic number.

Browsing the history of XLOG_PAGE_MAGIC, it used to be incremented by
one whenever the format changes, at least as long as it has been been in
xlog_internal.h. It started at 0xD05B in 2005, and was incremented by
one by each commit till 0xD062. But then the hot standby patch bumped it
to 0xD166, and on March 19th Simon changed it to 0x9002, and on March
28th to 0x9003.

Is there a plan somewhere on all that, or was it just random whacking?
It's a bit weird though that the commit on March 19th moved it
backwards. We don't guarantee backwards-compatibility, so any program
looking at the WAL needs to know not only minimum version it supports
but a list of all supported versions, so it's not catastrophic. Still
weird, though. It hasn't been like that for long yet, and not in any
released version, so we could still change it back to the original
scheme (0xD063).

0x9002 sounds like it means the 9.0 release, which might be a good
versioning scheme as well. If want to continue with that scheme, it
should be documented in the comments.

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

Re: WAL page magic number (was Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct)

Tom Lane <tgl@sss.pgh.pa.us> — 2010-04-28T15:11:54Z

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> Tom Lane wrote:
>> The correct way to reflect a change in WAL contents is to
>> bump the WAL page magic number.

> Browsing the history of XLOG_PAGE_MAGIC, it used to be incremented by
> one whenever the format changes, at least as long as it has been been in
> xlog_internal.h. It started at 0xD05B in 2005, and was incremented by
> one by each commit till 0xD062. But then the hot standby patch bumped it
> to 0xD166, and on March 19th Simon changed it to 0x9002, and on March
> 28th to 0x9003.

> Is there a plan somewhere on all that, or was it just random whacking?

Random whacking.  Simon seems to have decided to try to make the number
mean something, but it never meant anything before.  It's not a version
number, it's a magic identifier.  We might need to avoid certain values
to prevent collisions with other file formats, so I'm suspicious of
trying to make it match up with some expectation.

			regards, tom lane

Re: WAL page magic number (was Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct)

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-28T16:11:52Z

Tom Lane wrote:
> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>> Browsing the history of XLOG_PAGE_MAGIC, it used to be incremented by
>> one whenever the format changes, at least as long as it has been been in
>> xlog_internal.h. It started at 0xD05B in 2005, and was incremented by
>> one by each commit till 0xD062. But then the hot standby patch bumped it
>> to 0xD166, and on March 19th Simon changed it to 0x9002, and on March
>> 28th to 0x9003.
> 
>> Is there a plan somewhere on all that, or was it just random whacking?
> 
> Random whacking.  Simon seems to have decided to try to make the number
> mean something, but it never meant anything before.  It's not a version
> number, it's a magic identifier.  We might need to avoid certain values
> to prevent collisions with other file formats, so I'm suspicious of
> trying to make it match up with some expectation.

Ok, for better or worse, I whacked it back to the old series.

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-28T16:14:24Z
```
Ok, I've finally committed the patch, using wal_level as the name of the
GUC.

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Fujii Masao <masao.fujii@gmail.com> — 2010-04-29T02:33:32Z

On Thu, Apr 29, 2010 at 1:14 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
> Ok, I've finally committed the patch, using wal_level as the name of the
> GUC.

> ! 	if (InArchiveRecovery && XLogRequestRecoveryConnections)
> ! 	{
> ! 		if (ControlFile->wal_level < WAL_LEVEL_HOT_STANDBY)
> ! 			ereport(ERROR,
> ! 					(errmsg("recovery connections cannot start because wal_level was not set to 'hot_standby' on the WAL source server")));

I still have the complaint against the above check. Since the default value
of recovery_connections is TRUE, the users who need only archiving not
replication (i.e., wal_level is set to 'archive') are likely to often
see the failure
of the archive recovery by the above check.

Regards,

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-29T07:55:08Z

Fujii Masao wrote:
> On Thu, Apr 29, 2010 at 1:14 AM, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.com> wrote:
>> Ok, I've finally committed the patch, using wal_level as the name of the
>> GUC.
> 
>> ! 	if (InArchiveRecovery && XLogRequestRecoveryConnections)
>> ! 	{
>> ! 		if (ControlFile->wal_level < WAL_LEVEL_HOT_STANDBY)
>> ! 			ereport(ERROR,
>> ! 					(errmsg("recovery connections cannot start because wal_level was not set to 'hot_standby' on the WAL source server")));
> 
> I still have the complaint against the above check. Since the default value
> of recovery_connections is TRUE, the users who need only archiving not
> replication (i.e., wal_level is set to 'archive') are likely to often
> see the failure
> of the archive recovery by the above check.

Should we change the default to recovery_connections=off ? It is a quite
big change in default behavior over previous releases that hot standby
is enabled by default, so maybe that would be the right thing to do anyway.

Or maybe add a hint to the above, "disable hot standby by setting
recovery_connections=off, or set wal_level='hot_standby' in the primary"

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-29T08:18:57Z

On Thu, 2010-04-29 at 10:55 +0300, Heikki Linnakangas wrote:

> Should we change the default to recovery_connections=off ? It is a quite
> big change in default behavior over previous releases that hot standby
> is enabled by default, so maybe that would be the right thing to do anyway.
> 
> Or maybe add a hint to the above, "disable hot standby by setting
> recovery_connections=off, or set wal_level='hot_standby' in the primary"

I've been waiting for this suggestion, a clear knock-on effect from your
earlier changes. If we just have a static default its bad either way.

The most sensible way is to make the default act intelligently depending
upon what it finds in the control file. If WAL contains hot_standby
info, then recovery_connections should be on by default, though can be
turned off explicitly if required. If WAL does not contain hot_standby
info we then we throw a WARNING and continue as if recovery_connections
= off, or if recovery_connections is specified explicitly then we should
throw an ERROR so that the user knows it won't be possible to use this.
I think that means we'd need to change recovery_connections to have 2 or
3 values, but non-boolean:

recovery_connections = auto (default) | off
or
recovery_connections = auto (default) | on | off

and I would suggest removing it from postgresql.conf.sample

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-29T09:55:08Z

Simon Riggs wrote:
> On Thu, 2010-04-29 at 10:55 +0300, Heikki Linnakangas wrote:
> 
>> Should we change the default to recovery_connections=off ? It is a quite
>> big change in default behavior over previous releases that hot standby
>> is enabled by default, so maybe that would be the right thing to do anyway.
>>
>> Or maybe add a hint to the above, "disable hot standby by setting
>> recovery_connections=off, or set wal_level='hot_standby' in the primary"
> 
> I've been waiting for this suggestion, a clear knock-on effect from your
> earlier changes. If we just have a static default its bad either way.
> 
> The most sensible way is to make the default act intelligently depending
> upon what it finds in the control file. If WAL contains hot_standby
> info, then recovery_connections should be on by default, though can be
> turned off explicitly if required. If WAL does not contain hot_standby
> info we then we throw a WARNING and continue as if recovery_connections
> = off, or if recovery_connections is specified explicitly then we should
> throw an ERROR so that the user knows it won't be possible to use this.
> I think that means we'd need to change recovery_connections to have 2 or
> 3 values, but non-boolean:
> recovery_connections = auto (default) | off
> or
> recovery_connections = auto (default) | on | off
>

Seems overly complicated. It would be simpler to just set it 'off' by
default.

If it's 'off' by default, and you want to use hot standby, you will
notice quickly that the server doesn't accept connections, and you
switch it on. If it's 'on' (or 'auto'), you might not realize that your
standby server is accepting connections, when you only wanted to set it
up as a warm standby server for high availability.

The 'auto' mode just makes it more surprising. Connections might be
allowed at first, but when someone switches wal_level in the primary for
some reason, your reporting standby is up, but no longer allows
connections. And vice versa, if you set up a warm standby server for
high availability where you don't want to allow connections, and someone
flips the wal_level switch in the master, the standby suddenly starts
accepting connections.

I can't imagine a situation where "allow connections if the WAL allows
it" would be a sensible setting. If there is one, we could have an
'auto' mode, but not as the default.

> and I would suggest removing it from postgresql.conf.sample

-1. Why try to hide it?

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-29T10:19:52Z

On Thu, 2010-04-29 at 12:55 +0300, Heikki Linnakangas wrote:

> Seems overly complicated. 

If you turn wal_level = hot_standby in the master, then the slaves work,
unless you say otherwise. What is complicated about that?

> It would be simpler to just set it 'off' by
> default.

As a parameter, yes, it is simpler. 
The behaviour is not simplified by doing that.

I care about the behaviour.

> If it's 'off' by default, and you want to use hot standby, you will
> notice quickly that the server doesn't accept connections, and you
> switch it on. If it's 'on' (or 'auto'), you might not realize that your
> standby server is accepting connections, when you only wanted to set it
> up as a warm standby server for high availability.

That requires we hit the problem, alter the parameter and restart. We
don't need to do that at all. It could "Just Work".

> The 'auto' mode just makes it more surprising. Connections might be
> allowed at first, but when someone switches wal_level in the primary for
> some reason, your reporting standby is up, but no longer allows
> connections.

If you don't do this, what would happen on standby? Does it silently
stop applying changes after the point you turned it off, or does it
throw an ERROR.

> And vice versa, if you set up a warm standby server for
> high availability where you don't want to allow connections, and someone
> flips the wal_level switch in the master, the standby suddenly starts
> accepting connections.

Some people would regard that as a good thing. The whole point of
wal_level discussion was to avoid needing multiple switches to turn
something on. You are proposing exactly that here. Why is this
discussion different? Why should we not be able to set in just one
place.

If that behaviour concerns the DBA then they can turn it off explicitly.

> I can't imagine a situation where "allow connections if the WAL allows
> it" would be a sensible setting. If there is one, we could have an
> 'auto' mode, but not as the default.

I can. Simple, obvious behaviour. Turn it on in the master, works on the
standbys. We want people to use the feature, not fumble with it.

Just think "auto" means "linked to master". If you don't like it you can
turn it off.

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-29T10:46:32Z

Simon Riggs wrote:
> The whole point of
> wal_level discussion was to avoid needing multiple switches to turn
> something on.

No, the point of wal_level was to make the behavior easier to
understand, by uncoupling the level of WAL-logging from various other
switches, controling it directly and explicitly with a new GUC instead.
It added a new switch, but made the system as a whole easier to
understand and configure.

>> > I can't imagine a situation where "allow connections if the WAL allows
>> > it" would be a sensible setting. If there is one, we could have an
>> > 'auto' mode, but not as the default.
> 
> I can. Simple, obvious behaviour. Turn it on in the master, works on the
> standbys.

Yes, but when would you want that?

Here's the use cases I can think of:

purpose of the standby - do you want hot standby or not?
reporting - yes
offloading queries from master - yes
warm standby for high availability - no
offloading taking filesystem-level backups from master - no
offloading pg_dump from master - yes

All of those either want hot standby, or don't. What use case is there
for "enabled, if the required information is in the WAL"? If there is
one, it sure doesn't seem like the most common one. When you just want
hot standby to be on or off, it's weird to control that from the master.
Action at a distance.

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Robert Haas <robertmhaas@gmail.com> — 2010-04-29T11:15:56Z

On Thu, Apr 29, 2010 at 5:55 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
>> The most sensible way is to make the default act intelligently depending
>> upon what it finds in the control file. If WAL contains hot_standby
>> info, then recovery_connections should be on by default, though can be
>> turned off explicitly if required. If WAL does not contain hot_standby
>> info we then we throw a WARNING and continue as if recovery_connections
>> = off, or if recovery_connections is specified explicitly then we should
>> throw an ERROR so that the user knows it won't be possible to use this.
>> I think that means we'd need to change recovery_connections to have 2 or
>> 3 values, but non-boolean:
>> recovery_connections = auto (default) | off
>> or
>> recovery_connections = auto (default) | on | off
>>
>
> Seems overly complicated. It would be simpler to just set it 'off' by
> default.

I kind of agree with Simon on this one, except I would probably choose
to have just on and off and make on work like his auto.

In other words, recovery_connections=on means, give me recovery
connections if possible, otherwise don't worry about it.

I'd rather not have to change the default to recovery_connections=off
- that's one more parameter someone has to set properly.

...Robert

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-29T11:17:08Z

On Thu, 2010-04-29 at 13:46 +0300, Heikki Linnakangas wrote:

> purpose of the standby - do you want hot standby or not?

> reporting - yes
> offloading queries from master - yes
> offloading pg_dump from master - yes

These would work using the proposed default.

> warm standby for high availability - no
> offloading taking filesystem-level backups from master - no

These can be explicitly turned off.
You're presuming there is benefit in turning recovery_connections = off,
though it is perfectly valid to do those use cases with it on. There are
many ways to control connections, not just that switch. It will
certainly be easier to monitor the HA system by running queries against
it than not. Do you have any evidence there is benefit in the *typical*
case for turning the setting off? 

> All of those either want hot standby, or don't. What use case is there
> for "enabled, if the required information is in the WAL"? If there is
> one, it sure doesn't seem like the most common one. 

I think "I want it to just work" is fairly common.

> When you just want
> hot standby to be on or off, it's weird to control that from the master.
> Action at a distance.

That's the proposed default, not the only control. The standby can
override what the master says if it definitely doesn't want it, but is
smart enough to avoid silly configuration errors. Turning off wal_level
on the master *does* affect the standby, so pretending we have a
completely separate configuration option makes no sense for me.

Robert's point when he raised the wal_level discussion was about
reducing the number of parameter settings required to make this work. An
intelligent default will reduce level of configuration and allow us to
make Hot Standby work on the standby, out of the box, and that is worth
having.

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-29T11:19:27Z

Robert Haas wrote:
> I kind of agree with Simon on this one, except I would probably choose
> to have just on and off and make on work like his auto.
> 
> In other words, recovery_connections=on means, give me recovery
> connections if possible, otherwise don't worry about it.

If you're setting up a reporting server, and hot standby can't start,
the server is not functioning properly. I would like to get an error in
that case.

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Robert Haas <robertmhaas@gmail.com> — 2010-04-29T11:51:48Z

On Thu, Apr 29, 2010 at 7:19 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
> Robert Haas wrote:
>> I kind of agree with Simon on this one, except I would probably choose
>> to have just on and off and make on work like his auto.
>>
>> In other words, recovery_connections=on means, give me recovery
>> connections if possible, otherwise don't worry about it.
>
> If you're setting up a reporting server, and hot standby can't start,
> the server is not functioning properly. I would like to get an error in
> that case.

Presumably you will actually try connecting to it, no?

And what happens when someone changes the setting on the master from
hot_standby back to archive?  I'd rather have the reporting server
continue recovery without being able to accept connections rather than
die in its tracks.

I think this is a problem that should be solved by monitoring, rather
than by automatically taking the server down.

...Robert

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-29T11:55:33Z

Simon Riggs wrote:
> On Thu, 2010-04-29 at 13:46 +0300, Heikki Linnakangas wrote:
>> warm standby for high availability - no
>> offloading taking filesystem-level backups from master - no
> 
> These can be explicitly turned off.
> You're presuming there is benefit in turning recovery_connections = off,
> though it is perfectly valid to do those use cases with it on. There are
> many ways to control connections, not just that switch. It will
> certainly be easier to monitor the HA system by running queries against
> it than not. Do you have any evidence there is benefit in the *typical*
> case for turning the setting off? 

It depends on your exact configuration, but one typical one is that you
have a work-balancing router or pgbouncer sitting in front of the
servers, directing traffic to the server that's up and running. If the
standby starts accepting connections prematurely, the clients will be
incorrectly routed to the standby server and update operations will fail
(and SELECTs will return slightly delayed data).

>> All of those either want hot standby, or don't. What use case is there
>> for "enabled, if the required information is in the WAL"? If there is
>> one, it sure doesn't seem like the most common one. 
> 
> I think "I want it to just work" is fairly common.

You need quite a bit of set up anyway, flipping one more GUC hardly
makes a difference. There is less risk of oversight and accidental
misconfiguration if the admin makes a conscious decision to turn it on.

I'd like to scaremonger with the following fictional story:

An administrator sets up two PostgreSQL servers in a high availability
warm standby set up. Clients are set up to try to connect to both
servers, so that when failover happens, they will automatically
reconnect to the remaining server. wal_level is set to 'archive' in the
master, and all is well.

After running successfully for six months in production, a reporting
server is introduced to offload heavy queries from the mission-critical
OLTP server. wal_level is set to 'hot_standby' in the master to allow
read-only queries to be run against the reporting server, and the
reporting server is set up using the same WAL archive used for the warm
standby server. All seems to be running well, the admin logs in to the
application and clicks through a few screens to test it. A few hours
later a user rings and complains that he's getting a "cannot execute
INSERT in a read-only transaction" error. What happened, and why does it
work just fine when the admin tries the same?

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-29T12:19:10Z

On Thu, 2010-04-29 at 14:55 +0300, Heikki Linnakangas wrote:

> I'd like to scaremonger

Seems so.

recovery_connections was on by default and unanimous agreement until
recently and I don't want to change that now, just because a change
somewhere else appears to be forcing that but need not be so. It was
sensible to add a switch to turn HS off, but it should not be the
default, especially not one that requires a restart to enable a high
availability feature.

That is important in a feature that takes a while to "kick-in" and so
the user may patiently wait for it to come up and it never does.

I have no wish to repeat the situation that PostgreSQL requires a
restart to enable a feature, while other forks retain the ability to
enable the parameter without restart, as occurs with archive_mode, IIRC.

Perhaps we should not re-think wal_level if we've just moved the
parameter problem somewhere else?

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-29T12:54:21Z

Simon Riggs wrote:
> recovery_connections was on by default and unanimous agreement until
> recently and I don't want to change that now, just because a change
> somewhere else appears to be forcing that but need not be so.

We never really had that discussion, until now. It has always been 'on'
by default, and that has been useful to get more testing during the
development cycle, but it's not clear that's a good default for
real-life usage.

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Robert Haas <robertmhaas@gmail.com> — 2010-04-29T12:56:15Z

On Thu, Apr 29, 2010 at 7:55 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
> What happened,

I don't find that story very compelling because there are an infinite
number of ways to have high-availability not work if you start by
supposing that the person who sets them up doesn't test them properly
and verify that everything actually works as expected.  You could do
all sorts of things wrong in that case.

How about this one?  The administrator sets up a master and a slave.
She's heard about this new Hot Standby feature and so decides to
enable it on the slave just to play around with it.  Subsequently, she
takes a better job at another company and they hire a new
administrator, who thinks the Hot Standby WAL may be causing a
performance problem on the master, so he switches wal_mode to archive.
 Six months later the primary fails.

I think you can construct a scenario where just about any default
setting causes a problem, but I like the idea of having this enabled
by default, and I think it works fine if you just treat the case where
recovery_connections=on but wal_mode=archive as a LOG or WARNING
rather than an ERROR.

...Robert

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-29T13:00:44Z

Robert Haas wrote:
> On Thu, Apr 29, 2010 at 7:19 AM, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.com> wrote:
>> Robert Haas wrote:
>>> I kind of agree with Simon on this one, except I would probably choose
>>> to have just on and off and make on work like his auto.
>>>
>>> In other words, recovery_connections=on means, give me recovery
>>> connections if possible, otherwise don't worry about it.
>> If you're setting up a reporting server, and hot standby can't start,
>> the server is not functioning properly. I would like to get an error in
>> that case.
> 
> Presumably you will actually try connecting to it, no?

Sure. I guess it would be acceptable if 'on' meant 'on, if possible', as
long as 'off' is the default. Otherwise it's too surprising.

> And what happens when someone changes the setting on the master from
> hot_standby back to archive?  I'd rather have the reporting server
> continue recovery without being able to accept connections rather than
> die in its tracks.

As the code stands, if wal_level is switched from 'hot_standby' to
'archive' in the primary, and it's restarted, the standby will die.
There is currently no way to stop to accepting read-only connections
once they're allowed. It could restart with connections disallowed, but
it needs a restart.

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-29T13:07:03Z

Robert Haas wrote:
> How about this one?  The administrator sets up a master and a slave.
> She's heard about this new Hot Standby feature and so decides to
> enable it on the slave just to play around with it.  Subsequently, she
> takes a better job at another company and they hire a new
> administrator, who thinks the Hot Standby WAL may be causing a
> performance problem on the master, so he switches wal_mode to archive.
>  Six months later the primary fails.

Umm, I don't see the problem. For high availability purposes, the
standby works just as well with wal_mode='archive'. Or are you saying
that the standby was configured with recovery_connections='on', and
failed to start for those six months because hot standby could not be
enabled, and no-one noticed?

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Robert Haas <robertmhaas@gmail.com> — 2010-04-29T13:16:04Z

On Thu, Apr 29, 2010 at 9:00 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
> Robert Haas wrote:
>> On Thu, Apr 29, 2010 at 7:19 AM, Heikki Linnakangas
>> <heikki.linnakangas@enterprisedb.com> wrote:
>>> Robert Haas wrote:
>>>> I kind of agree with Simon on this one, except I would probably choose
>>>> to have just on and off and make on work like his auto.
>>>>
>>>> In other words, recovery_connections=on means, give me recovery
>>>> connections if possible, otherwise don't worry about it.
>>> If you're setting up a reporting server, and hot standby can't start,
>>> the server is not functioning properly. I would like to get an error in
>>> that case.
>>
>> Presumably you will actually try connecting to it, no?
>
> Sure. I guess it would be acceptable if 'on' meant 'on, if possible', as
> long as 'off' is the default. Otherwise it's too surprising.

I disagree.  I think on should mean 'on, if possible' and 'on' should
be the default.  I think that's how it was before this round of
changes - but maybe I'm mistaken?  It seems like it makes sense, at
any rate.

...Robert

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Robert Haas <robertmhaas@gmail.com> — 2010-04-29T13:16:26Z

On Thu, Apr 29, 2010 at 9:07 AM, Heikki Linnakangas
<heikki.linnakangas@enterprisedb.com> wrote:
> Robert Haas wrote:
>> How about this one?  The administrator sets up a master and a slave.
>> She's heard about this new Hot Standby feature and so decides to
>> enable it on the slave just to play around with it.  Subsequently, she
>> takes a better job at another company and they hire a new
>> administrator, who thinks the Hot Standby WAL may be causing a
>> performance problem on the master, so he switches wal_mode to archive.
>>  Six months later the primary fails.
>
> Umm, I don't see the problem. For high availability purposes, the
> standby works just as well with wal_mode='archive'. Or are you saying
> that the standby was configured with recovery_connections='on', and
> failed to start for those six months because hot standby could not be
> enabled, and no-one noticed?

Yep.

...Robert

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Dimitri Fontaine <dfontaine@hi-media.com> — 2010-04-29T13:36:42Z

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Apr 29, 2010 at 7:19 AM, Heikki Linnakangas
> <heikki.linnakangas@enterprisedb.com> wrote:
>> If you're setting up a reporting server, and hot standby can't start,
>> the server is not functioning properly. I would like to get an error in
>> that case.
>
> Presumably you will actually try connecting to it, no?
[...]
> I think this is a problem that should be solved by monitoring, rather
> than by automatically taking the server down.

+1 on both counts. Is that a +2?

Regards,
-- 
dim

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Tom Lane <tgl@sss.pgh.pa.us> — 2010-04-29T13:48:07Z

Simon Riggs <simon@2ndQuadrant.com> writes:
> recovery_connections was on by default and unanimous agreement until
> recently and I don't want to change that now,

Uh, it was on by default only because a lot of us hadn't noticed that.
I agree with Heikki's position here: it should not be on by default.

			regards, tom lane

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-29T15:24:10Z

On Thu, 2010-04-29 at 09:48 -0400, Tom Lane wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
> > recovery_connections was on by default and unanimous agreement until
> > recently and I don't want to change that now,
> 
> Uh, it was on by default only because a lot of us hadn't noticed that.
> I agree with Heikki's position here: it should not be on by default.

We're talking about the case where somebody has set up a standby
database. It's not like they happen on this accidentally.

* HS on by default, in the standby, via recovery_connections. 
* HS off by default, in the master, via wal_level. 

Overall, that *is* off by default. (Note: I said nothing about that).

We don't need it off *twice*, nor do we even need two switches.

Last week we had one switch and it was on by default, now we're looking
at two switches and off by default. I haven't yet heard a good reason
for the change being proposed here by Heikki. The use cases are rare, if
they truly exist at all. Monitoring the standby is much easier with HS
on, for example. 

What is the reason for the *extra* "off" switch? Why two? Why "off"
twice?

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Aidan Van Dyk <aidan@highrise.ca> — 2010-04-29T15:37:56Z

* Simon Riggs <simon@2ndQuadrant.com> [100429 11:24]:
> 
> What is the reason for the *extra* "off" switch? Why two? Why "off"
> twice?

Because I run a PITR slave off a WAL archive... And I'm going to
continue to try and use this setup... I expect to try and experiment
with wal-streaming too when everything is up to 9, and be a little upset
when the "old faithfull" backup I've got and am betting my job on
suddenly starts acting differentlyl, or accepting connections and
causing my db clients to start thowing errors because I'm trying to get
a streaming replication w/ hot standby up on a *different* slave...

It's all about the path of least *suprise*.  My "least suprise" would
have been that a similar config I have now with my PITR slaves would
pretty much still work, and not suddenly start accepting connections,
and even worse, at some later date when I've already verified that it
was doing what I expected with the configuration.

There has got to be *something* that tells me it is trying to allow
connectins during recovery, but failing...  And i think that just loging
a WARNING and continuing isn't enough, because I'll admit to having
upgraded PG and just "testing" it, not looking thoroughly through the
logs for any new messages that might peak my interest...

That said, since I follow -hackers, that "something" could be considered
this email thread, and I know I will be extra careful about my
deployment of PITR slaves w/ 9 wrt making sure I'm explicit in all the
settings I can find...

And I'll make sure I look more carefully at logs when deploying 9 as well
;-)

a.

-- 
Aidan Van Dyk                                             Create like a god,
aidan@highrise.ca                                       command like a king,
http://www.highrise.ca/                                   work like a slave.

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-29T15:40:14Z

Simon Riggs wrote:
> On Thu, 2010-04-29 at 09:48 -0400, Tom Lane wrote:
>> Simon Riggs <simon@2ndQuadrant.com> writes:
>>> recovery_connections was on by default and unanimous agreement until
>>> recently and I don't want to change that now,
>> Uh, it was on by default only because a lot of us hadn't noticed that.
>> I agree with Heikki's position here: it should not be on by default.
> 
> We're talking about the case where somebody has set up a standby
> database. It's not like they happen on this accidentally.
> 
> * HS on by default, in the standby, via recovery_connections. 
> * HS off by default, in the master, via wal_level. 
> 
> Overall, that *is* off by default. (Note: I said nothing about that).
> 
> We don't need it off *twice*, nor do we even need two switches.

wal_level is not supposed to control if Hot Standby is on or off. It
controls what information is written to WAL. If you have
wal_level='archive' and recovery_connections='on' in the standby, that's
a configuration error, just like setting wal_level='minimal' and
archive_mode='on'. You wanted to enable Hot Standby, but the information
required isn't in the WAL.

It's error-prone to control what happens in the standby from the master.
That's the "action at a distance" effect I mentioned earlier. The master
should be configured in the master, and each standby should configured
in the standby.

The right mental model is that wal_mode controls what is written to the
WAL. In fact, I might recommend always setting it to 'hot_standby'
instead of 'archive', even if you have no standbys and you're only doing
WAL archival - it's a lot easier to do PITR with hot standby. To control
whether hot standby is enabled in the standby, use recovery_connections.
And I'd prefer it to be off by default because off is the safer option.

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-29T16:02:23Z

On Thu, 2010-04-29 at 18:40 +0300, Heikki Linnakangas wrote:

> It's error-prone to control what happens in the standby from the master.
> That's the "action at a distance" effect I mentioned earlier. The master
> should be configured in the master, and each standby should configured
> in the standby.

Repeating the same thing when its been refuted doesn't help. What you
say has not been proposed.

If there is a case for HS-off-by-default, make it. If you want to change
code, arguing directly against your own position, mentioned many times,
we need a reason. How else can we know which argument of yours to
believe?

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-29T16:10:27Z

On Thu, 2010-04-29 at 11:37 -0400, Aidan Van Dyk wrote:
> * Simon Riggs <simon@2ndQuadrant.com> [100429 11:24]:
> > 
> > What is the reason for the *extra* "off" switch? Why two? Why "off"
> > twice?
> 
> Because I run a PITR slave off a WAL archive... And I'm going to
> continue to try and use this setup... I expect to try and experiment
> with wal-streaming too when everything is up to 9, and be a little upset
> when the "old faithfull" backup I've got and am betting my job on
> suddenly starts acting differentlyl, or accepting connections and
> causing my db clients to start thowing errors because I'm trying to get
> a streaming replication w/ hot standby up on a *different* slave...
> 
> It's all about the path of least *suprise*.  My "least suprise" would
> have been that a similar config I have now with my PITR slaves would
> pretty much still work, and not suddenly start accepting connections,
> and even worse, at some later date when I've already verified that it
> was doing what I expected with the configuration.

If people upgrade to 9.0 and then are surprised that the features listed
are actually available, I too would be surprised. 

If we all believe that these radical surprises are a problem, then we
should also turn off join removal and loads of other features in 9.0
that will also cause surprises.

HS is the most requested feature across multiple polls and *not* being
able to connect to it is likely to come as a huge surprise to many
people. HS is just as secure as the main database.

There is no big use case for "run with HS turned off". Everybody wants
it on, as long as it works and don't cause problems. So far, there is no
evidence to make anyone think that it would and I wish to avoid that
implication. 

Heikki previously argued strongly against having any switch at all, so
it could be turned on all the time. Nothing's changed.

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Aidan Van Dyk <aidan@highrise.ca> — 2010-04-29T16:17:55Z

* Simon Riggs <simon@2ndQuadrant.com> [100429 12:06]:
 
> Repeating the same thing when its been refuted doesn't help. What you
> say has not been proposed.
> 
> If there is a case for HS-off-by-default, make it. If you want to change
> code, arguing directly against your own position, mentioned many times,
> we need a reason. How else can we know which argument of yours to
> believe?

I'm not against HS being on-by-default.    But if it is, and the WAL
it's consuming doesn't have the HS-records by default, then I want PG to
consider that a problem, make sure I absolutely know it's a problem...

I agree with Heikki that the action-at-a-distance of HS
trying-to-work-but-maybe-not-this-time-depending-on-the-master is an
undesirable state...

Like everything else in PG, I'ld like it to "work completely", or tell
me there is a problem.  

That said, I'ld probalby be happy with PG 9 having a "default" config
of:

	wal_mode = hot_standby
	recovery_connections = on

Make it set to generate enough WAL and actually do recovery connections.

But also make the recover_connections boolean really mean what it s
called.  It's not called try_recovery_connections

a.


-- 
Aidan Van Dyk                                             Create like a god,
aidan@highrise.ca                                       command like a king,
http://www.highrise.ca/                                   work like a slave.

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-29T16:23:08Z

Simon Riggs wrote:
> On Thu, 2010-04-29 at 18:40 +0300, Heikki Linnakangas wrote:
> 
>> It's error-prone to control what happens in the standby from the master.
>> That's the "action at a distance" effect I mentioned earlier. The master
>> should be configured in the master, and each standby should configured
>> in the standby.
> 
> Repeating the same thing when its been refuted doesn't help. What you
> say has not been proposed.

I was responding to your mail where you said that there is two settings
for turning hot standby off, and asking why we need two. What I'm saying
is that you shouldn't think of wal_level as a setting to turn hot
standby on or off. It would be error-prone to control that from the
master. So there is only one setting to turn hot standby on/off,
recovery_connections in the standby.

> If there is a case for HS-off-by-default, make it. If you want to change
> code, arguing directly against your own position, mentioned many times,
> we need a reason. How else can we know which argument of yours to
> believe?

Now you lost me.

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Aidan Van Dyk <aidan@highrise.ca> — 2010-04-29T16:23:26Z

* Simon Riggs <simon@2ndQuadrant.com> [100429 12:14]:
 HS is the most requested feature across multiple polls and *not* being
> able to connect to it is likely to come as a huge surprise to many
> people. HS is just as secure as the main database.
> 
> There is no big use case for "run with HS turned off". Everybody wants
> it on, as long as it works and don't cause problems. So far, there is no
> evidence to make anyone think that it would and I wish to avoid that
> implication. 
> 
> Heikki previously argued strongly against having any switch at all, so
> it could be turned on all the time. Nothing's changed.

I'm not arguing against having it on by default.

What I'm against (and that's strong, it should probably be "prefer not
to have") is having it configured "on", but having it "not work".

If it's been configured to run in a state it can't, I would prefer it
didn't run, not that it ran, but in a slightly different state...

But I know that's just a preference... And one from an old-school unix
admin too...

a.

-- 
Aidan Van Dyk                                             Create like a god,
aidan@highrise.ca                                       command like a king,
http://www.highrise.ca/                                   work like a slave.

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Robert Haas <robertmhaas@gmail.com> — 2010-04-29T16:41:10Z

On Thu, Apr 29, 2010 at 12:17 PM, Aidan Van Dyk <aidan@highrise.ca> wrote:
> * Simon Riggs <simon@2ndQuadrant.com> [100429 12:06]:
>
>> Repeating the same thing when its been refuted doesn't help. What you
>> say has not been proposed.
>>
>> If there is a case for HS-off-by-default, make it. If you want to change
>> code, arguing directly against your own position, mentioned many times,
>> we need a reason. How else can we know which argument of yours to
>> believe?
>
> I'm not against HS being on-by-default.    But if it is, and the WAL
> it's consuming doesn't have the HS-records by default, then I want PG to
> consider that a problem, make sure I absolutely know it's a problem...

Nobody is proposing otherwise.  What Simon and I are proposing is that
if the master is configured to support HS, it comes up on the slave by
default without requiring additional configuration.  Now maybe that's
too much spooky action at a distance, but I suspect it IS the behavior
most people will want.  If Tom and Heikki get their way and change the
default behavior, it'll just mean (nearly) everyone flips one extra
configuration switch.

> I agree with Heikki that the action-at-a-distance of HS
> trying-to-work-but-maybe-not-this-time-depending-on-the-master is an
> undesirable state...
>
> Like everything else in PG, I'ld like it to "work completely", or tell
> me there is a problem.
>
> That said, I'ld probalby be happy with PG 9 having a "default" config
> of:
>
>        wal_mode = hot_standby
>        recovery_connections = on
>
> Make it set to generate enough WAL and actually do recovery connections.

That would be a bad idea - there's a significant performance penalty
from setting wal_level to anything other than minimal (just as there
is for turning on archive_mode in 8.4).

> But also make the recover_connections boolean really mean what it s
> called.  It's not called try_recovery_connections

Well, sure.  But setting work_mem to 1GB doesn't force the planner to
use a gigabyte of memory for every sort, either.  It just gives
permission.

...Robert

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Kevin Grittner <kevin.grittner@wicourts.gov> — 2010-04-29T16:48:47Z

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

> There is no big use case for "run with HS turned off".

There was mention of running the live database and a warm standby,
with a connection pooler pointed to both so that it could
automatically reconnect on failover.

> Everybody wants it on, as long as it works and don't cause
> problems.

I don't see any immediate use cases for our shop, unless it is the
only way to get WAL copied to our backup server through SR.  Franky,
I'd much rather have a WAL receiver which assembled the WAL file
segments (at the "archive" level of logging) as they arrived and
then fed them to our warm standby clusters.  (Of course, I've never
claimed to have a "typical" environment.)

I would be uncomfortable with a default of "auto" for a replica to
allow connections based on WAL file contents.  If someone explicitly
sets it to "auto", then it's up to them to understand the
implications.  IMO, If someone sets it to "on", they should get a
highly visible failure if it can't run in that mode.

-Kevin

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-29T16:55:22Z

Simon Riggs wrote:
> On Thu, 2010-04-29 at 11:37 -0400, Aidan Van Dyk wrote:
>> It's all about the path of least *suprise*.  My "least suprise" would
>> have been that a similar config I have now with my PITR slaves would
>> pretty much still work, and not suddenly start accepting connections,
>> and even worse, at some later date when I've already verified that it
>> was doing what I expected with the configuration.
> 
> If people upgrade to 9.0 and then are surprised that the features listed
> are actually available, I too would be surprised. 

Being available is not the same as being on by default.

> There is no big use case for "run with HS turned off". Everybody wants
> it on, as long as it works and don't cause problems. So far, there is no
> evidence to make anyone think that it would and I wish to avoid that
> implication. 

The use case I explained earlier exists, the one with a master and a
warm standby server with pgbouncer in front. And Aidan and me are human
beings, included in "everybody".

You know that there is cases where it causes problems in the standby,
even ignoring the possibility of bugs. For example, if you increase
max_connections in the master, the standby will abort. It's safer to run
with recovery_connections off if you don't need the feature.

> Heikki previously argued strongly against having any switch at all, so
> it could be turned on all the time. Nothing's changed.

I argued that we should always WAL-log the information required to
enable hot standby, on the grounds that the overhead is small. I.e. that
wal_level would be a two-state switch, either 'minimal' or
'hot_standby'. But I'm happy with how that is now.

But that's not what we're discussing now, don't confuse things. We're
talking about recovery_connections, not wal_level.

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-29T17:03:17Z

On Thu, 2010-04-29 at 11:48 -0500, Kevin Grittner wrote:
> Simon Riggs <simon@2ndQuadrant.com> wrote:
>  
> > There is no big use case for "run with HS turned off".
>  
> There was mention of running the live database and a warm standby,
> with a connection pooler pointed to both so that it could
> automatically reconnect on failover.

pgpool already copes by design and has been working against HS for
months.

If the connection pooler doesn't have some way of knowing the server is
a standby, then it will try to connect and fail, which presumably it
would retry.

This seems like a hypothetical connection pooler rather than a real one,
to me.

HS can continue to be used with people's existing Warm Standby setup,
just now you can read the database as well. Which is what everybody has
requested.

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Tom Lane <tgl@sss.pgh.pa.us> — 2010-04-29T17:03:42Z

Robert Haas <robertmhaas@gmail.com> writes:
> Nobody is proposing otherwise.  What Simon and I are proposing is that
> if the master is configured to support HS, it comes up on the slave by
> default without requiring additional configuration.  Now maybe that's
> too much spooky action at a distance, but I suspect it IS the behavior
> most people will want.  If Tom and Heikki get their way and change the
> default behavior, it'll just mean (nearly) everyone flips one extra
> configuration switch.

We already bought into the "one extra switch" penalty when we agreed to
invent a separate wal_level parameter.  The entire point of that was to
have more, but simpler-to-understand, parameters with fewer hidden
interactions.  Arguing that there are now too many knobs to twiddle
amounts to trying to revisit that discussion, which we don't have time
for now.

>> That said, I'ld probalby be happy with PG 9 having a "default" config
>> of:
>>    wal_mode = hot_standby
>>    recovery_connections = on

> That would be a bad idea - there's a significant performance penalty
> from setting wal_level to anything other than minimal (just as there
> is for turning on archive_mode in 8.4).

There is not only a performance penalty, but a reliability penalty.
Enabling these switches turns on a whole lot of code that, with all
due respect to those who have worked on it, is absolutely positively
guaranteed to be full of bugs.  Not all of which are going to be flushed
out during beta.  If we ship 9.0 with these things on by default, it
will result in an immediate reliability downgrade for installations that
are simply doing what they did before and not even interested in HS/SR.

Maybe by 9.1 or 9.2 it would be sensible to have some of this code
turned on by default.  But it is absolutely not in keeping with this
project's mindset or historical practice to enable it by default now.

			regards, tom lane

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Kevin Grittner <kevin.grittner@wicourts.gov> — 2010-04-29T17:16:12Z

Simon Riggs <simon@2ndQuadrant.com> wrote:
 
> Which is what everybody has requested.
 
You continue to use the term "everybody" rather loosely.
 
I don't begrudge people features they want, even when *I* don't need
them; but please don't take my lack of argument against adding this
feature as a silent request for it.  SR has a place in our shop,
especially if it can create traditional WAL file segments on the
receiving end. (I understand that will not be a built-in feature for
9.0, but perhaps there will be a pgfoundry project or some such.) HS
is no use to us, and I would rather not pay a performance penalty or
take the risks of exercising complex new code paths because others
need it.
 
-Kevin

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Robert Haas <robertmhaas@gmail.com> — 2010-04-29T17:20:03Z

On Thu, Apr 29, 2010 at 1:03 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> That said, I'ld probalby be happy with PG 9 having a "default" config
>>> of:
>>>        wal_mode = hot_standby
>>>        recovery_connections = on
>
>> That would be a bad idea - there's a significant performance penalty
>> from setting wal_level to anything other than minimal (just as there
>> is for turning on archive_mode in 8.4).
>
> There is not only a performance penalty, but a reliability penalty.
> Enabling these switches turns on a whole lot of code that, with all
> due respect to those who have worked on it, is absolutely positively
> guaranteed to be full of bugs.  Not all of which are going to be flushed
> out during beta.  If we ship 9.0 with these things on by default, it
> will result in an immediate reliability downgrade for installations that
> are simply doing what they did before and not even interested in HS/SR.

This is a pretty good argument, and Heikki's argument just upthread
that a mismatched max_connections setting could bollix things is an
even better one.  So I'm now changing my mind and thinking this should
be off by default, also.

...Robert

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-29T17:37:56Z

On Thu, 2010-04-29 at 13:03 -0400, Tom Lane wrote:

> There is not only a performance penalty

I've asked for evidence that recovery is any slower as a result of HS.
If we had some, I'm guessing we'd be tuning rather than discussing this.

> Enabling these switches turns on a whole lot of code that, with all
> due respect to those who have worked on it, is absolutely positively
> guaranteed to be full of bugs. 

I understand that view and don't take offence at all, thank you for your
concern.

I wish to take seriously the possibility you may be correct, so I want
to squash unfounded rumours that will scare people into keeping HS
turned off, because that will keep the bugs in, rather than flush them
out so we can fix them.

Can we keep the default=on during beta and collect evidence before
making a final decision at release?

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-29T17:39:47Z

On Thu, 2010-04-29 at 12:16 -0500, Kevin Grittner wrote:
> Simon Riggs <simon@2ndQuadrant.com> wrote:
>  
> > Which is what everybody has requested.
>  
> You continue to use the term "everybody" rather loosely.

Sorry, that was imprecise, I didn't mean to exaggerate.

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Greg Smith <greg@2ndquadrant.com> — 2010-04-29T17:42:14Z

Aidan Van Dyk wrote:
> It's all about the path of least *suprise*.  My "least suprise" would
> have been that a similar config I have now with my PITR slaves would
> pretty much still work, and not suddenly start accepting connections,
> and even worse, at some later date when I've already verified that it
> was doing what I expected with the configuration.
>   

The first time I setup a system to test HS, when I got to the point 
where the slave was up and running as a standard warm standby, I 
expected there to be something else I had to do in order for it to be 
available for queries.  When I fired up psql and I was able to run 
queries without doing anything extra, I was surprised--but it was that 
fun, everything just works when I expected it to be harder than that 
kind of surprise.

One of the reasons the version number was bumped up to 9.0 was to put 
people on warning that they should not assume their old setups would 
port forward without behavioral changes.  The fact that existing 
warm-standby server users will be surprised to find they can run queries 
without doing anything special could be considered under that banner.  
If you feel that's not obvious enough, that could argue for more 
prominent documentation of that fact, rather than turning it off.  The 
idea that it should be made harder to enable just to protect the 
expectations current users, and therefore introduce yet another place 
where PostgreSQL is less friendly to get started with than it could be, 
is backwards from the perspective of making things as easy as possible 
for new users.

Arguing from a usability standpoint needs to consider both new and 
existing user requirements, and those are quite opposed to one another 
in terms of what default makes more sense IMHO.  Now, if the argument is 
from the perspective of "this adds performance/reliability issues that 
weren't there before", and those go away if the feature is disabled by 
default, that's a respectable and indisputable reason to do so.

-- 
Greg Smith  2ndQuadrant US  Baltimore, MD
PostgreSQL Training, Services and Support
greg@2ndQuadrant.com   www.2ndQuadrant.us

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Joshua D. Drake <jd@commandprompt.com> — 2010-04-29T17:42:40Z

On Thu, 2010-04-29 at 13:03 -0400, Tom Lane wrote:

> There is not only a performance penalty, but a reliability penalty.
> Enabling these switches turns on a whole lot of code that, with all
> due respect to those who have worked on it, is absolutely positively
> guaranteed to be full of bugs.  Not all of which are going to be flushed
> out during beta.  If we ship 9.0 with these things on by default, it
> will result in an immediate reliability downgrade for installations that
> are simply doing what they did before and not even interested in HS/SR.
> 
> Maybe by 9.1 or 9.2 it would be sensible to have some of this code
> turned on by default.  But it is absolutely not in keeping with this
> project's mindset or historical practice to enable it by default now.
> 
> 			regards, tom lane

+1

Joshua D. Drake


> 


-- 
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564
Consulting, Training, Support, Custom Development, Engineering

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-29T17:57:23Z

Simon Riggs wrote:
> Can we keep the default=on during beta and collect evidence before
> making a final decision at release?

That's tempting to get more (inadvertent) testing of HS, but I don't
think it would be fair to the beta testers. The expectation is that
what's in beta is what's in the release, unless some new issue pops up.

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-29T18:00:40Z

On Thu, 2010-04-29 at 19:55 +0300, Heikki Linnakangas wrote:

> And Aidan and me are human
> beings, included in "everybody".

Yes, that was too loose. I dislike that kind of argument and I'm sorry
that slipped in here.

> You know that there is cases where it causes problems in the standby,
> even ignoring the possibility of bugs. For example, if you increase
> max_connections in the master, the standby will abort.

That behaviour was suggested by you. I don't think its anywhere near
necessary that it does that and would like to remove that restriction.
The likelihood we'll ever run out of slots is small even with large
increases in max_connections. Example: if we increase from 100 to 500
we'd only hit the limit if we had all 500 connections on the master
simultaneously running write transactions with an average 12
subtransactions each.

> It's safer to run
> with recovery_connections off if you don't need the feature.

Presumably your advice is also that people should not run with Streaming
Replication if they don't need that feature? And that we should also
have an enable_joinremoval flag so the risk it poses can be minimized?
You didn't argue this way with regard to vacuum/FSM in 8.4, which was
much more critical; we're just talking about a standby server.

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Kevin Grittner <kevin.grittner@wicourts.gov> — 2010-04-29T18:01:32Z

Simon Riggs <simon@2ndQuadrant.com> wrote:
 
> I've asked for evidence that recovery is any slower as a result of
> HS.
 
Can you quantify the impact on the number of bytes written to WAL
when switching from the archiving level to the hot standby level?
 
-Kevin

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-29T18:12:34Z

On Thu, 2010-04-29 at 13:20 -0400, Robert Haas wrote:

> This is a pretty good argument, and Heikki's argument just upthread
> that a mismatched max_connections setting could bollix things is an
> even better one.  So I'm now changing my mind and thinking this should
> be off by default, also.

What Heikki hasn't said is that setting max_prepared_transactions
incorrectly has always caused a fatal error at failover, and still does
so now when HS is disabled. If these concerns were real ones, then that
situation would not have existed for years and would not still do so. 

9.0 without HS enabled is fairly untested on the standby, so it's not
clear to me turning it off makes you any safer.

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Tom Lane <tgl@sss.pgh.pa.us> — 2010-04-29T18:13:35Z

Simon Riggs <simon@2ndQuadrant.com> writes:
> Can we keep the default=on during beta and collect evidence before
> making a final decision at release?

I'd be for doing that if it didn't have adverse effects on the use of
pg_start_backup (which is where we started this thread).  But it would
be to get more testing, not because we would consider beta results as
even possibly giving sufficient confidence to justify shipping 9.0.0
with the default = on.

I realize that you've sweated blood over a long period to get this
stuff in there, and if I were you I'd probably be wishing it could
be on by default too.  But from a project management standpoint
it's just way too risky to even consider.  HS/SR at this point have
to be seen as being about as trustworthy as the Windows port was in
8.0.0.  No DBA is going to be happy with that stuff getting turned
on unless he specifically asks for it.

Maybe in 9.1 or 9.2, but not in 9.0.

			regards, tom lane

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Kevin Grittner <kevin.grittner@wicourts.gov> — 2010-04-29T18:19:39Z

Simon Riggs <simon@2ndQuadrant.com> wrote:
 
> 9.0 without HS enabled is fairly untested on the standby, so it's
> not clear to me turning it off makes you any safer.
 
I think you've just made the strongest possible case for not
defaulting it on during beta testing.
 
-Kevin

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-29T18:24:17Z

On Thu, 2010-04-29 at 13:01 -0500, Kevin Grittner wrote:
> Simon Riggs <simon@2ndQuadrant.com> wrote:
>  
> > I've asked for evidence that recovery is any slower as a result of
> > HS.
>  
> Can you quantify the impact on the number of bytes written to WAL
> when switching from the archiving level to the hot standby level?

Sure, done that a few times.

Extra WAL data is written for these actions, listed in order of
increasing size

* commit records contain a variable length list of
relcacheinvalidations, mostly applies only to DDL
* one extra WAL record in most VACUUMs, fairly small, optimised away in
some cases
* a transaction issues more than 64 subtransactions it will issue a
record approx 256 bytes plus header
* one extra WAL record every checkpoint, containing a full current
snapshot's worth of running xids 100-400 bytes typically, could go up
from there to 4000 bytes in very extreme write workloads that also have
many, many subtransactions

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-29T18:27:27Z

Simon Riggs wrote:
> On Thu, 2010-04-29 at 19:55 +0300, Heikki Linnakangas wrote:
>> It's safer to run
>> with recovery_connections off if you don't need the feature.
> 
> Presumably your advice is also that people should not run with Streaming
> Replication if they don't need that feature? 

Umm, yes. Why would you bother to set it up if you don't need it?

> And that we should also
> have an enable_joinremoval flag so the risk it poses can be minimized?
> You didn't argue this way with regard to vacuum/FSM in 8.4, which was
> much more critical; we're just talking about a standby server.

This is getting bizarre...

I wasn't really following the join removal discussions, but it seems
much safer and less complex. And it would not have been feasible to have
both implementations of FSM around and have the user to choose. I have
been relieved by the lack of bug reports on the new FSM, that was a big
change that impacted all installations.

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-29T18:34:55Z

Simon Riggs wrote:
> What Heikki hasn't said is that setting max_prepared_transactions
> incorrectly has always caused a fatal error at failover, and still does
> so now when HS is disabled.

Hmm, good point. That should probably be documented somewhere. Now that
we WAL-log the max_prepared_xacts value, I suppose we could throw a
WARNING too.

> If these concerns were real ones, then that
> situation would not have existed for years and would not still do so. 

There's very few people using two-phase commit out there, and even less
so in a standby configuration. I wouldn't draw any conclusions from that.

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

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Joshua D. Drake <jd@commandprompt.com> — 2010-04-29T18:39:47Z

On Thu, 2010-04-29 at 21:27 +0300, Heikki Linnakangas wrote:


> I wasn't really following the join removal discussions, but it seems
> much safer and less complex. And it would not have been feasible to have
> both implementations of FSM around and have the user to choose. I have
> been relieved by the lack of bug reports on the new FSM, that was a big
> change that impacted all installations.

I agree that Simon's argument doesn't hold a lot of water in comparison
to the features he is citing. I say leave it on for Beta just so we can
hopefully get some inherit testing but other than that, its off for
release.

Joshua D. Drake


-- 
PostgreSQL.org Major Contributor
Command Prompt, Inc: http://www.commandprompt.com/ - 503.667.4564
Consulting, Training, Support, Custom Development, Engineering

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Tom Lane <tgl@sss.pgh.pa.us> — 2010-04-29T18:44:35Z

"Kevin Grittner" <Kevin.Grittner@wicourts.gov> writes:
> Simon Riggs <simon@2ndQuadrant.com> wrote:
>> 9.0 without HS enabled is fairly untested on the standby, so it's
>> not clear to me turning it off makes you any safer.

> I think you've just made the strongest possible case for not
> defaulting it on during beta testing.

Yes.  If we've had it on so far, flipping it at the start of beta seems
sensible from a test coverage standpoint.  We will certainly have a
reasonable number of beta testers who are interested in the feature and
turn it on to test it; but we need to make sure the "off" position isn't
accidentally broken.

			regards, tom lane

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Kevin Grittner <kevin.grittner@wicourts.gov> — 2010-04-29T18:47:41Z

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

> [most significant extra WAL logging for hot standby is:]
> * one extra WAL record every checkpoint, containing a full current
> snapshot's worth of running xids 100-400 bytes typically, could go
> up from there to 4000 bytes in very extreme write workloads that
> also have many, many subtransactions

The most convincing evidence that there's no significant performance
hit for those not needing the feature (and not a bad thing from a
testing point of view anyway) would be for someone to run some
benchmarks comparing the archive and hot_standby logging levels,
with no archive script or SR.  If that hasn't been done yet, maybe
you could find somebody who knows his way around pgbench-tools, who
could construct a reasonable test and produce graphs and all that
cool stuff.  ;-)  IMO, isolating the cost of generating and writing
the extra WAL would be very valuable, and it would be reassuring to
know that it worked with a large number of clients without exposing
any dire race conditions.

-Kevin

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Tom Lane <tgl@sss.pgh.pa.us> — 2010-04-29T20:18:41Z

Okay, this thread seems to be running out of steam, and we are running
out of hours before the beta1 wrap.  I am going to take it on my own
authority to do the following:

* rename recovery_connections to hot_standby (and the underlying
variable from XLogRequestRecoveryConnections to something like
EnableHotStandby);

* change its default value to 'off'.

I believe the only other code changes needed for beta1 are the
pg_start/stop_backup error check changes that Heikki proposed at
<4BD953A6.70409@enterprisedb.com>.  If he doesn't commit that
within an hour or two, I will.

This train is leaving the station.

			regards, tom lane

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Robert Haas <robertmhaas@gmail.com> — 2010-04-29T20:33:01Z

On Thu, Apr 29, 2010 at 4:18 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> This train is leaving the station.

An enthusiastic +1 for train departure.

...Robert

Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct

Simon Riggs <simon@2ndquadrant.com> — 2010-04-29T20:36:48Z

On Thu, 2010-04-29 at 16:18 -0400, Tom Lane wrote:

> * rename recovery_connections to hot_standby (and the underlying
> variable from XLogRequestRecoveryConnections to something like
> EnableHotStandby);

OK

> * change its default value to 'off'.

OK

-- 
 Simon Riggs           www.2ndQuadrant.com

Re: WAL page magic number (was Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct)

Bruce Momjian <bruce@momjian.us> — 2010-04-30T16:43:03Z

Heikki Linnakangas wrote:
> Tom Lane wrote:
> > Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
> >> Browsing the history of XLOG_PAGE_MAGIC, it used to be incremented by
> >> one whenever the format changes, at least as long as it has been been in
> >> xlog_internal.h. It started at 0xD05B in 2005, and was incremented by
> >> one by each commit till 0xD062. But then the hot standby patch bumped it
> >> to 0xD166, and on March 19th Simon changed it to 0x9002, and on March
> >> 28th to 0x9003.
> > 
> >> Is there a plan somewhere on all that, or was it just random whacking?
> > 
> > Random whacking.  Simon seems to have decided to try to make the number
> > mean something, but it never meant anything before.  It's not a version
> > number, it's a magic identifier.  We might need to avoid certain values
> > to prevent collisions with other file formats, so I'm suspicious of
> > trying to make it match up with some expectation.
> 
> Ok, for better or worse, I whacked it back to the old series.

Agreed.  Making it match the version would be odd because you then have
t bump it for every major release.

-- 
  Bruce Momjian  <bruce@momjian.us>        http://momjian.us
  EnterpriseDB                             http://enterprisedb.com

Re: WAL page magic number (was Re: Re: [COMMITTERS] pgsql: Make CheckRequiredParameterValues() depend upon correct)

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> — 2010-04-30T18:43:03Z

Bruce Momjian wrote:
> Heikki Linnakangas wrote:
>> Tom Lane wrote:
>>> Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:
>>>> Browsing the history of XLOG_PAGE_MAGIC, it used to be incremented by
>>>> one whenever the format changes, at least as long as it has been been in
>>>> xlog_internal.h. It started at 0xD05B in 2005, and was incremented by
>>>> one by each commit till 0xD062. But then the hot standby patch bumped it
>>>> to 0xD166, and on March 19th Simon changed it to 0x9002, and on March
>>>> 28th to 0x9003.
>>>> Is there a plan somewhere on all that, or was it just random whacking?
>>> Random whacking.  Simon seems to have decided to try to make the number
>>> mean something, but it never meant anything before.  It's not a version
>>> number, it's a magic identifier.  We might need to avoid certain values
>>> to prevent collisions with other file formats, so I'm suspicious of
>>> trying to make it match up with some expectation.
>> Ok, for better or worse, I whacked it back to the old series.
> 
> Agreed.  Making it match the version would be odd because you then have
> t bump it for every major release.

Not necessarily. It would make sense to me to just keep it the same if
there has been no changes. The magic number would indicate the first
version that used that format.

I'm not advocating a switch to that system, but it would be sensible as
well.

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