On Wed, Oct 5, 2011 at 02:36, gabrielle <gor...@gmail.com> wrote: > This review was compiled from a PDXPUG group review (Dan Colish, Mark > Wong, Brent Dombrowski, and Gabrielle Roth).
Hi, thanks for the review! > - Regression test requires plpythonu; it needs to work without that. The patch contains no regression tests and -- as far as I can tell -- cannot be reliably tested in the current pg_regress framework. The plpythonu line is just an example to demonstrate the patch output. > - The doc comment 'pgstat_get_backend_current_activity' doesn't match > the function name 'pgstat_get_crashed_backend_activity'. Oops, copy-paste error. :) > - 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.) That was deliberate. As far as I've seen, in Postgres source, complex expressions are usually lined up to the starting parentheses, unless the expression is too long, in which case it's aligned to the 78-character right margin. I decided to use this because splitting the expression on yet one more line wouldn't improve code readability. Or have I misunderstood the coding style? > - Unknown is used a lot (see > http://developer.postgresql.org/pgdocs/postgres/error-style-guide.html#AEN94099) The string "(unknown)" in postmaster.c was there before me, I didn't change that. The other instance of "unknown" in the comment for ascii_safe_strncpy I believe expresses the function quite well -- the function doesn't know and doesn't care what the input encoding is. > We had some questions about ascii_safe_strncpy: > - It doesn't convert just unknown encodings, it converts all > encodings. Is that intentional? Technically we always "know" the right encoding -- the query is in the backend's database encoding. The point here is being simple and foolproof -- not introducing unnecessary amounts of code into the postmaster. Since ASCII characters are valid in any encoding, we only keep ASCII characters and throw away the rest. This was added in response to concerns that ereport might attempt to convert the string to another encoding and fail -- because the command string may be corrupt or because postmaster's encoding doesn't match the backend's encoding. And while this concern doesn't seem to apply with current code, it's still prudent to add it to pre-empt future changes and to protect the log file from potentially corrupt strings. See: http://archives.postgresql.org/pgsql-hackers/2011-09/msg00418.php > - Is there an existing function that already does this? The bytea->text conversion (byteaout function) does something similar when bytea_output='escape', but it doesn't preserve newline/tab characters and it doesn't currently exist as an independent function. It also uses the outdated octal representation. Regards, Marti -- Sent via pgsql-hackers mailing list (email@example.com) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers