pgsql: Report which WAL sync method we are trying to change *to* when it
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
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
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