Re: [PATCH] Log crashed backend's query v2

From: gabrielle <gorthx(at)gmail(dot)com>
To: pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Cc: Marti Raudsepp <marti(at)juffo(dot)org>, Mark Wong <markwkm(at)gmail(dot)com>, Brent Dombrowski <brent(dot)dombrowski(at)gmail(dot)com>, Dan Colish <dan(at)unencrypted(dot)org>
Subject: Re: [PATCH] Log crashed backend's query v2
Date: 2011-10-04 23:36:17
Message-ID: CAHRs-_c9Pzr6ANb=grMTqEPXHD15Uurmu1voAzDRfwEJi_QpgQ@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

This review was compiled from a PDXPUG group review (Dan Colish, Mark
Wong, Brent Dombrowski, and Gabrielle Roth).

--

We all agree this is a really useful feature. The patch applies
cleanly to the current git master with git apply, it's in context
diff, and does what it's supposed to do on Ubuntu, OSX, and gentoo.

We found a few problems with it, and we'd like to see the patch
resubmitted with the following changes:
Tests:
- Regression test requires plpythonu; it needs to work without that.

Docs:
- The doc comment 'pgstat_get_backend_current_activity' doesn't match
the function name 'pgstat_get_crashed_backend_activity'.

Project coding guidelines:
- There are some formatting problems, such as all newlines at the same
indent level need to line up. (The author mentioned "not [being]
happy with the indenting depth", so I think this is not a surprise.)
- Wayward tabs, line 2725 in pgstat.c specifically
- Unknown is used a lot (see
http://developer.postgresql.org/pgdocs/postgres/error-style-guide.html#AEN94099)

We had some questions about ascii_safe_strncpy:
- It doesn't convert just unknown encodings, it converts all
encodings. Is that intentional?
- Is there an existing function that already does this?

Looking forward to the next revision!

gabrielle (on behalf of PDXPUG)

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Bruce Momjian 2011-10-05 00:25:04 Re: Bug with pg_ctl -w/wait and config-only directories
Previous Message Royce Ausburn 2011-10-04 22:45:38 Re: [PATCH] Unremovable tuple monitoring (was: Addition of some trivial auto vacuum logging)