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

On Wed, Oct 5, 2011 at 02:36, gabrielle <gorthx(at)gmail(dot)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