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)
