Thread: pgsql: Report which WAL sync method we are trying to change *to* when it
pgsql: Report which WAL sync method we are trying to change *to* when it
From
mha@postgresql.org (Magnus Hagander)
Date:
Log Message:
-----------
Report which WAL sync method we are trying to change *to* when it fails,
not which one we had before (that worked, and thus is completley irrelevant)
Modified Files:
--------------
pgsql/src/backend/access/transam:
xlog.c (r1.304 -> r1.305)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/transam/xlog.c?r1=1.304&r2=1.305)
On Mon, 2008-05-12 at 14:27 +0000, Magnus Hagander wrote: > Log Message: > ----------- > Report which WAL sync method we are trying to change *to* when it fails, > not which one we had before (that worked, and thus is completley irrelevant) Interesting perspective. If it breaks, I'd rather be able to put it back the way it was than regret in technicolour that my new choice was a bad one. ;-) Not everybody keeps a change log. Could we report both? -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com
Re: pgsql: Report which WAL sync method we are trying to change *to* when it
From
Magnus Hagander
Date:
Simon Riggs wrote: > On Mon, 2008-05-12 at 14:27 +0000, Magnus Hagander wrote: > > Log Message: > > ----------- > > Report which WAL sync method we are trying to change *to* when it > > fails, not which one we had before (that worked, and thus is > > completley irrelevant) > > Interesting perspective. > > If it breaks, I'd rather be able to put it back the way it was than > regret in technicolour that my new choice was a bad one. ;-) Well, the message itself indicated that it was the new one... > Not everybody keeps a change log. > > Could we report both? Yes, we could easily do that if we want to. But - this is not the error you get when you try to set it. It's the error you get when you try to *use* it. And really, it's a "should never happen" error. (The reason it happens this time is due to another bug). So I don't think doing so would actually help your case - it's already covered elsewhere in the code where we'll rollback the setting when you try to change it instead of PANICing. //Magnus
Magnus Hagander <magnus@hagander.net> writes:
> Simon Riggs wrote:
>> Could we report both?
> Yes, we could easily do that if we want to.
It would be entirely silly to do so, since (a) the old value hasn't been
changed if we fail here, and (b) it's irrelevant to the nature of the
error.
What's also a bit silly is using PANIC for this report --- AFAICS plain
old ERROR is sufficient, since if we fail here we have not yet modified
any persistent state. Just because it's a "can't happen" condition
doesn't justify making it a PANIC.
regards, tom lane
On Mon, 2008-05-12 at 15:26 -0400, Tom Lane wrote: > Magnus Hagander <magnus@hagander.net> writes: > > Simon Riggs wrote: > >> Could we report both? > > > Yes, we could easily do that if we want to. > > It would be entirely silly to do so, since (a) the old value hasn't been > changed if we fail here, and (b) it's irrelevant to the nature of the > error. That's reasonable. If it is impossible to set it to an impossible/failing value then that is even better. Magnus seems to say it is possible to set this and then have it fail later when it is used. Not sure which is correct. -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com
Re: pgsql: Report which WAL sync method we are trying to change *to* when it
From
Magnus Hagander
Date:
Simon Riggs wrote: > On Mon, 2008-05-12 at 15:26 -0400, Tom Lane wrote: > > Magnus Hagander <magnus@hagander.net> writes: > > > Simon Riggs wrote: > > >> Could we report both? > > > > > Yes, we could easily do that if we want to. > > > > It would be entirely silly to do so, since (a) the old value hasn't > > been changed if we fail here, and (b) it's irrelevant to the nature > > of the error. > > That's reasonable. If it is impossible to set it to an > impossible/failing value then that is even better. > > Magnus seems to say it is possible to set this and then have it fail > later when it is used. Not sure which is correct. It shouldn't ever happen. It happened here because there was a bug in my original patch, that has now been fixed. So unless there are more bugs in it, it is now back to can't happen. //Magnus
Simon Riggs <simon@2ndquadrant.com> writes:
> Magnus seems to say it is possible to set this and then have it fail
> later when it is used. Not sure which is correct.
Per his comment just now, I think he'd gotten confused between
assign_xlog_sync_method (which sets the value) and issue_xlog_fsync
(which uses it).
regards, tom lane
On Mon, 2008-05-12 at 21:49 +0200, Magnus Hagander wrote: > Simon Riggs wrote: > > On Mon, 2008-05-12 at 15:26 -0400, Tom Lane wrote: > > > Magnus Hagander <magnus@hagander.net> writes: > > > > Simon Riggs wrote: > > > >> Could we report both? > > > > > > > Yes, we could easily do that if we want to. > > > > > > It would be entirely silly to do so, since (a) the old value hasn't > > > been changed if we fail here, and (b) it's irrelevant to the nature > > > of the error. > > > > That's reasonable. If it is impossible to set it to an > > impossible/failing value then that is even better. > > > > Magnus seems to say it is possible to set this and then have it fail > > later when it is used. Not sure which is correct. > > It shouldn't ever happen. It happened here because there was a bug in > my original patch, that has now been fixed. So unless there are more > bugs in it, it is now back to can't happen. OK, good. Just checking it won't ever happen to me ;-) (and if it does, I have a backout plan). -- Simon Riggs 2ndQuadrant http://www.2ndQuadrant.com