Proposed Windows-specific change: Enable crash dumps (like core files)

On 10/04/2010 07:50 AM, Craig Ringer wrote:

- If the crash dump handler is enabled by setting the GUC,
all backends register the handler during startup or (if it
proves practical) when the GUC is changed.

- When the handler is triggered by the OS trapping an unhandled
exception, it loads dbghelp.dll, writes the appropriate dump
format to the hardcoded path, and terminates the process.

What is the performance impact of doing that? Specifically, how does it
affect backend startup time?

cheers

andrew

On Mon, Oct 4, 2010 at 12:50 PM, Craig Ringer
<craig@postnewspapers.com.au> wrote:

- Always compile the crash dump feature in builds, so every build that
 goes to a user is capable of crash dumps. Because a suitable version
 of dbghelp.dll has been included in Windows XP and above, it shouldn't
 be necessary to ship the DLL with Pg, though the license permits that.

We probably would want to - the version that comes with Windows is
somewhat cut down, and MSDN recommends using the more recent versions
from the debugging tools.

Any idea of the performance overhead?

(BCC'd Dave Page because of his involvement in Windows & Windows Pg
support).

That explains why it bypassed my filters :-)

--
Dave Page
Blog: http://pgsnake.blogspot.com
Twitter: @pgsnake

EnterpriseDB UK: http://www.enterprisedb.com
The Enterprise Postgres Company

On 4/10/2010 8:06 PM, Andrew Dunstan wrote:

On 10/04/2010 07:50 AM, Craig Ringer wrote:

- If the crash dump handler is enabled by setting the GUC,
all backends register the handler during startup or (if it
proves practical) when the GUC is changed.

- When the handler is triggered by the OS trapping an unhandled
exception, it loads dbghelp.dll, writes the appropriate dump
format to the hardcoded path, and terminates the process.

What is the performance impact of doing that? Specifically, how does it
affect backend startup time?

Without testing I can't say for sure.

My expection based on how the handler works would be: near-zero, about
as expensive as registering a signal handler, plus the cost of reading
the GUC and doing one string compare to test the value. When disabled,
it's just the GUC test.

Is there a better mechanism to use for features that're going to be
unused the great majority of the time? Perhaps something that does
require a server restart, but doesn't have any cost at all when disabled?

--
Craig Ringer

Tech-related writing at http://soapyfrogs.blogspot.com/

On 4/10/2010 8:09 PM, Dave Page wrote:

Any idea of the performance overhead?

A GUC read and string compare when off, and (untested as yet) I expect
little more when on.

Thinking about it, a possibly better alternative is to ship it as a
library as is done with the pl/pgsql debugger, and use (I think)
shared_preload_libraries to load it when desired. That way there's zero
cost if disabled, though a somewhat higher cost if enabled.

Hmm. That'll let me put a test version together that'll work with 9.0 as
well, so that seems like a no-brainer really, it's just a better way to
do it. Time for a Pg-on-windows build, yay.

--
Craig Ringer

Tech-related writing at http://soapyfrogs.blogspot.com/

On 04.10.2010 15:21, Craig Ringer wrote:

Thinking about it, a possibly better alternative is to ship it as a
library as is done with the pl/pgsql debugger, and use (I think)
shared_preload_libraries to load it when desired. That way there's zero
cost if disabled, though a somewhat higher cost if enabled.

Hmm. That'll let me put a test version together that'll work with 9.0 as
well, so that seems like a no-brainer really, it's just a better way to
do it. Time for a Pg-on-windows build, yay.

If it's not a lot of code, it's better to have it built-in always.
Loading extra libraries in debug-mode could lead to heisenbugs.

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

On 4/10/2010 8:27 PM, Heikki Linnakangas wrote:

On 04.10.2010 15:21, Craig Ringer wrote:

Thinking about it, a possibly better alternative is to ship it as a
library as is done with the pl/pgsql debugger, and use (I think)
shared_preload_libraries to load it when desired. That way there's zero
cost if disabled, though a somewhat higher cost if enabled.

Hmm. That'll let me put a test version together that'll work with 9.0 as
well, so that seems like a no-brainer really, it's just a better way to
do it. Time for a Pg-on-windows build, yay.

If it's not a lot of code, it's better to have it built-in always.
Loading extra libraries in debug-mode could lead to heisenbugs.

Good point. The built-in approach would also permit it to be turned on
in an already-running server, which would be nice as for those fun
only-shows-up-in-production bugs where you may not want to restart Pg.

I'll chuck together a library version first, though, so it can be tested
easily on existing installs of Pg 9.0. Compiling Pg on Windows isn't as
quick and easy as on *nix so that should help for testing/consideration.
If people are happy with the results I'll put together a patch to
integrate it directly instead of using a library.

--
Craig Ringer

Tech-related writing at http://soapyfrogs.blogspot.com/

On 4/10/2010 8:27 PM, Heikki Linnakangas wrote:

On 04.10.2010 15:21, Craig Ringer wrote:

Thinking about it, a possibly better alternative is to ship it as a
library as is done with the pl/pgsql debugger, and use (I think)
shared_preload_libraries to load it when desired. That way there's zero
cost if disabled, though a somewhat higher cost if enabled.

Hmm. That'll let me put a test version together that'll work with 9.0 as
well, so that seems like a no-brainer really, it's just a better way to
do it. Time for a Pg-on-windows build, yay.

If it's not a lot of code, it's better to have it built-in always.
Loading extra libraries in debug-mode could lead to heisenbugs.

It turns out that the basic minidumps are small (21kb here) and very
fast to generate. It may well be worth leaving it enabled all the time
after all. I just need to time how long the handler registration takes -
though as LOAD of the current handler implemented as a contrib module
takes 2.1ms, and LOAD of an module with an empty _PG_init() also takes
2.1ms, I'm guessing "not long".

I've attached an early version for people to play with if anyone's
interested. It currently contains a bunch of elog() messages to report
on its progress - though I'm not at all sure elog() should be used in
the final version given that Pg is crashing at the time the handler is
called and there's no guarantee elog() is safe to call. It also doesn't
offer any way to set the dump type yet, it's always the minimal dump
with exception info, registers and stack only. Finally, a "crashme"
function is included to trigger a reliable unhandled exception on
demand, for initial testing.

Usage with Pg built from source on Windows:

- Drop crashdump.c and the Makefile in contrib/crashdump
- Run build.bat and install.bat
- Create a "crashdumps" directory inside your datadir, and make
sure the server has read/write/create permission on it.
- add 'crashdump' to shared_preload_libraries in postgresql.conf
- Start the server as normal
- Start psql and issue:
-- CREATE OR REPLACE FUNCTION crashdump_crashme() RETURNS void
AS '$libdir/crashdump','crashdump_crashme' LANGUAGE C;
-- SELECT crashdump_crashme();

Your backend should crash. In the error log (and, depending on how the
buffering works out, maybe in psql) you should see:

WARNING: loading dll
WARNING: loading dll try 1, 00000000
WARNING: loading dll try 2, 73880000
WARNING: pdump: 738C70D8
WARNING: Generating dumppath
WARNING: Generated dumppath
WARNING: dumping to path: crashdumps\backend.dmp
WARNING: outfile: 000000B0
WARNING: about to dump
NOTICE: crashdump: wrote crash dump to crashdumps\postgres-4341.mdmp

Once you have the dump file, you can fire up windbg from the Debugging
Tools for Windows and use File->Open Crash Dump to open it. The .excr
command will report what the exception that caused the crash was,
producing output like this:

0:000> .ecxr
eax=00000000 ebx=00000000 ecx=02a1e290 edx=73831014 esi=02a1e188 edi=00c3f798
eip=738313b2 esp=00c3f724 ebp=02a1e280 iopl=0 nv up ei pl zr na pe nc
cs=001b ss=0023 ds=0023 es=0023 fs=003b gs=0000 efl=00010246
crashdump!crashdump_crashme+0x2:
738313b2 c70001000000 mov dword ptr [eax],1 ds:0023:00000000=????????

and if possible opening the postgresql source file the crash happened in
to the appropriate line:

Datum
crashdump_crashme(PG_FUNCTION_ARGS)
{
int * ptr = NULL;
*ptr = 1; <--- highlighted
return NULL;
}

If you're using Visual Studio Professional (not the free Express
edition, unfortunately) you should also be able to debug the crash dump
that way. I don't have it to test with.

--
Craig Ringer

Tech-related writing at http://soapyfrogs.blogspot.com/

OK, it's pretty much ready for proper testing now. If a few people are
happy with the results and think it's a good idea I'll chuck it in the
commitfest app.

As built, the crash dump handler works with a stock PostgreSQL 9.0
(release build) as shipped in EDB's installer. Just drop crashdump.dll
in lib\, optionally pop the dbghelp.dll redist in bin\, add 'crashdump'
to shared_preload_libraries, and crash some backends however you feel
like doing so.

The current build of crashdump.dll for release versions of PostgreSQL
9.0 on 32-bit Windows is here:

http://www.postnewspapers.com.au/~craig/webfiles/crashdump.dll

If folks are happy with how this works, all it needs is:

- Consideration of whether elog should be used or not. I'm inclined to
suggest using elog to tell the user about the dump, but only after
the crash dump has been written successfully.

- Comments on whether this should be left as a loadable module, or
integerated into core so it loads early in backend startup. The latter
will permit crash dumping of early backend startup problems, and will
have tiny overhead because there's no DLL to find and load. OTOH, it's
harder to pull out if somehow something breaks.

I'd want to start with loadable module in shared_preload_libraries
and if that proves useful, only then consider integrating in core.
I'm way too bad a programmer to want my code anywhere near Pg's core
without plenty of real world testing.

- (Maybe) a method to configure the crash dump type. I'm not too sure
it's necessary given the set of dump flags I've landed up with,
so I'd leave this be unless it proves to be necessary in real-world
testing.

Remember that with these crash dumps the user only has to email the
(~4MB in my tests) crash dump. They don't need debugging tools or the
ability to use them, only the recipient does.

I'm using a tweaked set of minidump flags now, to generate considerably
bigger dumps (around 4MB for the configuration I'm testing) that contain
pretty much everything except shared memory contents, the contents of
memory mapped files, and the loaded read-only code segments of the
executables and DLLs. You can see the results of using it to debug that
autovac crash at the end of this mail.

When testing the script provided by Andrea Peri, when the autovacuum
worker crashes, the message:

2010-10-05 22:23:44 WST 2040 WARNING: crashdump: wrote crash dump to crashdumps\postgres-2040.mdmp

is emitted before the process resumes crashing out as it would've normally.

Opening and displaying that dump in windbg shows a useful stack trace
from the crashing process, and it's possible to examine the state of
local/global variables etc.

Craig Ringer wrote:

On 4/10/2010 8:06 PM, Andrew Dunstan wrote:

On 10/04/2010 07:50 AM, Craig Ringer wrote:

- If the crash dump handler is enabled by setting the GUC,
all backends register the handler during startup or (if it
proves practical) when the GUC is changed.

- When the handler is triggered by the OS trapping an unhandled
exception, it loads dbghelp.dll, writes the appropriate dump
format to the hardcoded path, and terminates the process.

What is the performance impact of doing that? Specifically, how does it
affect backend startup time?

Without testing I can't say for sure.

My expection based on how the handler works would be: near-zero, about
as expensive as registering a signal handler, plus the cost of reading
the GUC and doing one string compare to test the value. When disabled,
it's just the GUC test.

Is there a better mechanism to use for features that're going to be
unused the great majority of the time? Perhaps something that does
require a server restart, but doesn't have any cost at all when disabled?

We definately had trouble producing crash dumps caused by the 128 return
code problem, so I can see great value in this, if it can be done
simply. I wonder if the 128-exit would have produced a crash file.

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

+ It's impossible for everything to be true. +

On Tue, Oct 5, 2010 at 17:21, Craig Ringer <craig@postnewspapers.com.au> wrote:

OK, it's pretty much ready for proper testing now. If a few people are happy
with the results and think it's a good idea I'll chuck it in the commitfest
app.

As built, the crash dump handler works with a stock PostgreSQL 9.0 (release
build) as shipped in EDB's installer. Just drop crashdump.dll in lib\,
optionally pop the dbghelp.dll redist in bin\, add 'crashdump' to
shared_preload_libraries, and crash some backends however you feel like
doing so.

The current build of crashdump.dll for release versions of PostgreSQL 9.0 on
32-bit Windows is here:

 http://www.postnewspapers.com.au/~craig/webfiles/crashdump.dll

If folks are happy with how this works, all it needs is:

- Consideration of whether elog should be used or not. I'm inclined to
 suggest using elog to tell the user about the dump, but only after
 the crash dump has been written successfully.

- Comments on whether this should be left as a loadable module, or
 integerated into core so it loads early in backend startup. The latter
 will permit crash dumping of early backend startup problems, and will
 have tiny overhead because there's no DLL to find and load. OTOH, it's
 harder to pull out if somehow something breaks.

 I'd want to start with loadable module in shared_preload_libraries
 and if that proves useful, only then consider integrating in core.
 I'm way too bad a programmer to want my code anywhere near Pg's core
 without plenty of real world testing.

- (Maybe) a method to configure the crash dump type. I'm not too sure
 it's necessary given the set of dump flags I've landed up with,
 so I'd leave this be unless it proves to be necessary in real-world
 testing.

Finally getting to looking at this one - sorry about the very long delay.

I agree with Heikki's earlier comment that it's better to have this
included in the backend - but that's obviously not going to happen for
already-released versions. I'd therefor advocate a contrib module for
existing versions, and then in core for 9.1+.

We should then have an option to turn it off (on by default). But we
don't need to pay the overhead on every backend startup - we could
just map the value into the parameter shared memory block, and require
a full postmaster restart in order to change it.

Do we want to backpatch it into contrib/? Adding a new module there
seems kind of wrong - probably better to keep the source separate and
just publish the DLL files for people who do debugging?

Looking at the code:
* Why do we need to look at differnt versions of dbghelp.dll? Can't we
always use the one with Windows? And in that case, can't we just use
compile-time linking, do we need to bother with DLL loading at all?

* Per your comment about using elog() - a good idea in that case is to
use write_stderr(). That will send the output to stderr if there is
one, and otherwise the eventlog (when running as service).

* And yes, in the crash handler, we should *definitely* not use elog().

* On Unix, the core file is dropped in the database directory, we
don't have a separate directory for crashdumps. If we want to be
consistent, we should do that here too. I do think that storing them
in a directory like "crashdumps" is better, but I just wanted to raise
the comment.

* However, when storing it in crashdumps, I think the code would need
to create that directory if it does not exist, doesn't it?

* Right now, we overwrite old crashdumps. It is well known that PIDs
are recycled pretty quickly on Windows - should we perhaps dump as
postgres-<pid>-<sequence>.mdmp when there is a filename conflict?

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Mon, Nov 22, 2010 at 6:37 AM, Magnus Hagander <magnus@hagander.net> wrote:

Do we want to backpatch it into contrib/? Adding a new module there
seems kind of wrong - probably better to keep the source separate and
just publish the DLL files for people who do debugging?

If this works without changes to core, I see little reason not to
back-patch it into contrib. Our primary concern with back-patching is
to avoid doing things that might break existing installations, but if
there aren't any core changes, I don't really see how that can be an
issue here. It seems to me that it's probably simpler for us and our
users to keep the debugging tools together with our main tree.

However, I am not clear what benefit we get from moving this into core
in 9.1. If it's still going to require a full postmaster restart, the
GUC you have to change may as well be shared_preload_libraries as a
new one.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Robert Haas <robertmhaas@gmail.com> writes:

However, I am not clear what benefit we get from moving this into core
in 9.1. If it's still going to require a full postmaster restart, the
GUC you have to change may as well be shared_preload_libraries as a
new one.

+1. I am not in favor of randomly repackaging functionality, unless
there's some clear benefit gained by doing so. In this case it seems
like something that could and should remain at arm's length forever,
so a contrib module is the ideal packaging.

regards, tom lane

On Mon, Nov 22, 2010 at 9:49 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Robert Haas <robertmhaas@gmail.com> writes:

However, I am not clear what benefit we get from moving this into core
in 9.1.  If it's still going to require a full postmaster restart, the
GUC you have to change may as well be shared_preload_libraries as a
new one.

+1.  I am not in favor of randomly repackaging functionality, unless
there's some clear benefit gained by doing so.  In this case it seems
like something that could and should remain at arm's length forever,
so a contrib module is the ideal packaging.

Now, if we could make this so low-overhead that it doesn't need a
switch, that would be a good argument for moving it into core, at
least to my way of thinking. But if it's something that needs to be
enabled with a postmaster restart anyway, meh.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On mån, 2010-11-22 at 12:37 +0100, Magnus Hagander wrote:

Do we want to backpatch it into contrib/?

It's not a bug fix or an upgrading aid, so no.

Peter Eisentraut <peter_e@gmx.net> writes:

On mån, 2010-11-22 at 12:37 +0100, Magnus Hagander wrote:

Do we want to backpatch it into contrib/?

It's not a bug fix or an upgrading aid, so no.

I'm less than thrilled about back-patching this, too. It seems to fly
in the face of all our historical practice.

If you drop the bit about back-patching, then I don't particularly care
whether it goes into core or contrib for 9.1 --- whichever packaging
makes the most sense is fine.

regards, tom lane

On Mon, Nov 22, 2010 at 16:33, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Peter Eisentraut <peter_e@gmx.net> writes:

On mån, 2010-11-22 at 12:37 +0100, Magnus Hagander wrote:

Do we want to backpatch it into contrib/?

It's not a bug fix or an upgrading aid, so no.

I'm less than thrilled about back-patching this, too.  It seems to fly
in the face of all our historical practice.

If you drop the bit about back-patching, then I don't particularly care
whether it goes into core or contrib for 9.1 --- whichever packaging
makes the most sense is fine.

My suggestion in the first place was not to backpatch it - I just
wanted to get peoples opinions. I'm perfectly happy if we keep it
somewhere else for the time being - as long as we make the binaries
easily available. But that can go on the wiki for example.

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

On Mon, Nov 22, 2010 at 15:15, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Nov 22, 2010 at 6:37 AM, Magnus Hagander <magnus@hagander.net> wrote:

Do we want to backpatch it into contrib/? Adding a new module there
seems kind of wrong - probably better to keep the source separate and
just publish the DLL files for people who do debugging?

If this works without changes to core, I see little reason not to
back-patch it into contrib.  Our primary concern with back-patching is
to avoid doing things that might break existing installations, but if
there aren't any core changes, I don't really see how that can be an
issue here.  It seems to me that it's probably simpler for us and our
users to keep the debugging tools together with our main tree.

However, I am not clear what benefit we get from moving this into core
in 9.1.  If it's still going to require a full postmaster restart, the
GUC you have to change may as well be shared_preload_libraries as a
new one.

on-by-default is what we gain. I think that's fairly big...

--
 Magnus Hagander
 Me: http://www.hagander.net/
 Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus@hagander.net> writes:

on-by-default is what we gain. I think that's fairly big...

Only if that's actually what we want, which is far from clear in this
corner. There are good reasons why most Linux distros configure daemons
not to dump core by default. It's annoying when we are trying to debug
a Postgres crash, but that doesn't mean the reasons aren't good.

regards, tom lane

On Mon, Nov 22, 2010 at 10:17 AM, Peter Eisentraut <peter_e@gmx.net> wrote:

On mån, 2010-11-22 at 12:37 +0100, Magnus Hagander wrote:

Do we want to backpatch it into contrib/?

It's not a bug fix or an upgrading aid, so no.

I don't see why an upgrading aid would be worthy of back-patching, but
not a debugging aid. I'd certainly prioritize those in the other
order.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company