Thread: pgsql: Make CheckRequiredParameterValues() depend upon correct

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)

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

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

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

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

            regards, tom lane

On Fri, Apr 23, 2010 at 3:57 PM, Simon Riggs <sriggs@postgresql.org> wrote:
> Log Message:
> -----------
> Make CheckRequiredParameterValues() depend upon correct combination
> of parameters. Fix bug report by Robert Haas that error message and
> hint was incorrect if wrong mode parameters specified on master.
> Internal changes only. Proposals for parameter simplification on
> master/primary still under way.

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

...Robert

On Fri, 2010-04-23 at 16:04 -0400, Tom Lane wrote:
> sriggs@postgresql.org (Simon Riggs) writes:
> > Log Message:
> > -----------
> > Make CheckRequiredParameterValues() depend upon correct combination
> > of parameters. Fix bug report by Robert Haas that error message and
> > hint was incorrect if wrong mode parameters specified on master.
> > Internal changes only. Proposals for parameter simplification on
> > master/primary still under way.
>
> > Modified Files:
> > --------------
> >     pgsql/src/backend/access/transam:
> >         xlog.c (r1.401 -> r1.402)
> >         (http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/transam/xlog.c?r1=1.401&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

On Fri, 2010-04-23 at 16:05 -0400, Robert Haas wrote:
> On Fri, Apr 23, 2010 at 3:57 PM, Simon Riggs <sriggs@postgresql.org> wrote:
> > Log Message:
> > -----------
> > Make CheckRequiredParameterValues() depend upon correct combination
> > of parameters. Fix bug report by Robert Haas that error message and
> > hint was incorrect if wrong mode parameters specified on master.
> > Internal changes only. Proposals for parameter simplification on
> > master/primary still under way.
>
> This is (1) premature, (2) not the consensus position, and unless I
> mistaken,

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

I will revoke.

--
 Simon Riggs           www.2ndQuadrant.com

Simon Riggs <simon@2ndQuadrant.com> writes:
> No intention of doing that. This change allows people to see what the
> dependency actually is once the bug has been fixed. Change needs to
> start from here, not from where we were before.

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

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

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

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

            regards, tom lane

On Fri, Apr 23, 2010 at 4:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> No intention of doing that. This change allows people to see what the
>> dependency actually is once the bug has been fixed. Change needs to
>> start from here, not from where we were before.
>
> Well, actually, now that I've looked at the patch I think it's starting
> from a fundamentally wrong position anyway.  Checkpoint records are a
> completely wrong mechanism for transmitting this data to slaves, because
> a checkpoint is emitted *after* we do something, not *before* we do it.
> In particular it's ludicrous to be looking at shutdown checkpoints to
> try to determine whether the subsequent WAL will meet the slave's
> requirements.  There's no connection at all between what the GUC state
> was at shutdown and what it might be after starting again.
>
> A design that might work is
> (1) store the active value of wal_mode in pg_control (but NOT as part of
> the last-checkpoint-record image).
> (2) invent a new WAL record type that is transmitted when we change
> wal_mode.

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

...Robert

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

> There's no connection at all between what the GUC state
> was at shutdown and what it might be after starting again.
>
> A design that might work is
> (1) store the active value of wal_mode in pg_control (but NOT as part of
> the last-checkpoint-record image).
> (2) invent a new WAL record type that is transmitted when we change
> wal_mode.
>
> Then, slaves could check whether the master's wal_mode is high enough
> by looking at pg_control when they start plus any wal_mode_change
> records they come across.

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

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

Please.

--
 Simon Riggs           www.2ndQuadrant.com

Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Apr 23, 2010 at 4:44 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> A design that might work is
>> (1) store the active value of wal_mode in pg_control (but NOT as part of
>> the last-checkpoint-record image).
>> (2) invent a new WAL record type that is transmitted when we change
>> wal_mode.

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

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

            regards, tom lane