pgsql: rm_cleanup functions need to be allowed to write WAL entries.
Log Message:
-----------
rm_cleanup functions need to be allowed to write WAL entries. This oversight
appears to explain the recent reports of "PANIC: cannot make new WAL entries
during recovery".
Modified Files:
--------------
pgsql/src/backend/access/transam:
xlog.c (r1.345 -> r1.346)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/transam/xlog.c?r1=1.345&r2=1.346)
On Fri, 2009-08-07 at 19:29 +0000, Tom Lane wrote:
Log Message:
-----------
rm_cleanup functions need to be allowed to write WAL entries. This oversight
appears to explain the recent reports of "PANIC: cannot make new WAL entries
during recovery".Modified Files:
--------------
pgsql/src/backend/access/transam:
xlog.c (r1.345 -> r1.346)
(http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/src/backend/access/transam/xlog.c?r1=1.345&r2=1.346)
I was just working on a patch after the comments yesterday, hadn't
noticed you'd committed.
The first chunk is exactly as I was going to suggest. Make an explicit
state change in the startup process.
Resetting it back seems fragile, since in crash recovery we call it
again almost immediately during CreateCheckPoint(). That only works if
LocalSetXLogInsertAllowed() has no side effects. I understand Heikki's
wish to have safeguards in place, so we should document that
LocalSetXLogInsertAllowed() can be executed twice without problem.
--
Simon Riggs www.2ndQuadrant.com
Simon Riggs <simon@2ndQuadrant.com> writes:
Resetting it back seems fragile, since in crash recovery we call it
again almost immediately during CreateCheckPoint(). That only works if
LocalSetXLogInsertAllowed() has no side effects. I understand Heikki's
wish to have safeguards in place, so we should document that
LocalSetXLogInsertAllowed() can be executed twice without problem.
Done.
My initial thought had actually been to get rid of the use of
LocalSetXLogInsertAllowed inside CreateCheckPoint, since with this
patch we are calling it from the same bit of code that is about
to call CreateCheckPoint --- leaving the flag set throughout that
bit would be fine. However that would only work as intended if
the checkpoint were done locally; if somehow we'd launched the
bgwriter and the checkpoint request got sent over there, it'd fail.
I don't believe this is currently possible during a crash recovery
scenario, but on the whole it seemed more fragile to do it that way
than in the code as-committed. In principle, at least, it seems
possible that the rm_cleanup and checkpoint actions could be done
in different processes, and this setup preserves the freedom to
let that happen.
regards, tom lane
On Sat, 2009-08-08 at 12:46 -0400, Tom Lane wrote:
Simon Riggs <simon@2ndQuadrant.com> writes:
Resetting it back seems fragile, since in crash recovery we call it
again almost immediately during CreateCheckPoint(). That only works if
LocalSetXLogInsertAllowed() has no side effects. I understand Heikki's
wish to have safeguards in place, so we should document that
LocalSetXLogInsertAllowed() can be executed twice without problem.Done.
My initial thought had actually been to get rid of the use of
LocalSetXLogInsertAllowed inside CreateCheckPoint, since with this
patch we are calling it from the same bit of code that is about
to call CreateCheckPoint --- leaving the flag set throughout that
bit would be fine. However that would only work as intended if
the checkpoint were done locally; if somehow we'd launched the
bgwriter and the checkpoint request got sent over there, it'd fail.
I don't believe this is currently possible during a crash recovery
scenario, but on the whole it seemed more fragile to do it that way
than in the code as-committed.
OK
In principle, at least, it seems
possible that the rm_cleanup and checkpoint actions could be done
in different processes, and this setup preserves the freedom to
let that happen.
Good.
I want to move in the direction of having two cleanup routines, one
executed before recovery ends and one done afterwards, so it can write
WAL. Perhaps these would be called rm_makesafe() and rm_repair(). Rough
thinking at this stage.
The rm_repair() would execute in a separate process once we're up.
--
Simon Riggs www.2ndQuadrant.com
Simon Riggs <simon@2ndQuadrant.com> writes:
I want to move in the direction of having two cleanup routines, one
executed before recovery ends and one done afterwards, so it can write
WAL. Perhaps these would be called rm_makesafe() and rm_repair(). Rough
thinking at this stage.
The rm_repair() would execute in a separate process once we're up.
Er, what's the point of that? It would make life tremendously harder
for resource managers, which could no longer rely on tracking their
state locally within the startup process. And AFAICS there is no
benefit to be had, compared to the existing plan of letting backends
run while the startup process is still active.
regards, tom lane
On Sun, 2009-08-09 at 11:41 -0400, Tom Lane wrote:
Simon Riggs <simon@2ndQuadrant.com> writes:
I want to move in the direction of having two cleanup routines, one
executed before recovery ends and one done afterwards, so it can write
WAL. Perhaps these would be called rm_makesafe() and rm_repair(). Rough
thinking at this stage.The rm_repair() would execute in a separate process once we're up.
Er, what's the point of that?
Rebuilding damaged indexes automatically, rather than barfing. I regard
that as a long term extension of crash recovery to bring a database back
to a usable state.
--
Simon Riggs www.2ndQuadrant.com
Simon Riggs <simon@2ndQuadrant.com> writes:
On Sun, 2009-08-09 at 11:41 -0400, Tom Lane wrote:
Er, what's the point of that?
Rebuilding damaged indexes automatically, rather than barfing. I regard
that as a long term extension of crash recovery to bring a database back
to a usable state.
Having crash recovery auto-rebuild indexes it thinks are damaged seems
to me to be a pretty horrid idea. Just for starters, it would overwrite
forensic evidence about the cause of the damage. A DBA might not wish
the rebuild to happen *right then* in any case.
regards, tom lane
Tom Lane wrote:
Simon Riggs <simon@2ndQuadrant.com> writes:
On Sun, 2009-08-09 at 11:41 -0400, Tom Lane wrote:
Er, what's the point of that?
Rebuilding damaged indexes automatically, rather than barfing. I regard
that as a long term extension of crash recovery to bring a database back
to a usable state.Having crash recovery auto-rebuild indexes it thinks are damaged seems
to me to be a pretty horrid idea. Just for starters, it would overwrite
forensic evidence about the cause of the damage. A DBA might not wish
the rebuild to happen *right then* in any case.
Are hash indexes going to need auto-rebuild, or can we make them
WAL-safe eventually?
--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com
+ If your life is a hard drive, Christ can be your backup. +