Thread: [PATCH] Log crashed backend's query v2
On Sat, Sep 24, 2011 at 22:57, Marti Raudsepp <marti@juffo.org> wrote: > However, I now realize that it does make sense to write a separate > simpler function for the crashed backend case with no > vbeentry->st_changecount check loops, no checkUser, etc. That would be > more robust and easier to review. I implemented this now, but I'm not convinced anymore that it's the right way to go. I'm duplicating some amount of code that could be subject to bitrot in the future since this code path won't be excercised often. But I'll let the reviewers decide. Is there a sane way to regression-test backend crashes? > I propsed replacing non-ASCII characters with '?' earlier. This is also in. I created a new function in backend/utils/adt/ascii.c. It didn't quite fit in because all other functions in this file are dealing with Datums, but I couldn't find a better place. (I'm still not sure what "adt" means) Regards, Marti
Attachment
On Sep28, 2011, at 00:19 , Marti Raudsepp wrote: > (I'm still not sure what "adt" means) I always assumed it stood for "abstract data type". Most of the files in this directory seem to correspond to an SQL-leveldata type like intX, varchar, tsquery, ..., and contain the I/O functions for that type, plus some supporting operationsand functions. Over time, it seems that this directory was also used for SQL-level functions not directly related to a single type, likewindowfuncs.c and pgstatfuncs.c. The fact that ri_triggers.c lives there also might be a relict from times where you'dcreate FK constraint with CREATE CONSTRAINT TRIGGER and specified one of the functions from ri_triggers.c as the procedureto execute. best regards, Florian Pflug
Florian Pflug <fgp@phlo.org> writes: > On Sep28, 2011, at 00:19 , Marti Raudsepp wrote: >> (I'm still not sure what "adt" means) > I always assumed it stood for "abstract data type". Yeah, that's what I think too. Over time it's been used to hold most code that is a SQL-callable function, many of which are not directly connected to any SQL datatype. Not sure if it's worth trying to clean that up. Another annoying thing is that "adt" should probably have been directly under src/backend/ --- dropping it under utils/ seems just weird for a category that is going to hold a ton of code. (I had once had some hope that git would allow us to move code around more easily, but my experiments with back-patching after code movement have convinced me that it doesn't work any better for that than CVS. So I'm not in favor of massive code rearrangements just to make the source tree structure cleaner.) regards, tom lane
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)
On Wed, Oct 5, 2011 at 02:36, gabrielle <gorthx@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
On Wed, Oct 5, 2011 at 02:36, gabrielle <gorthx@gmail.com> wrote: > This review was compiled from a PDXPUG group review (Dan Colish, Mark > Wong, Brent Dombrowski, and Gabrielle Roth). Whaat, you marked the patch as "Returned with Feedback" based on this review? The only obvious change I need to make in response to your feedback is the function name fix in a comment. Most points are incorrect: there's no regression test in this patch and no requirement of plpythonu. I didn't introduce any new messages with the text "unknown". The behavior of ascii_safe_strncpy is deliberate and was implemented from feedback on the first patch version. What's left is the indentation alignment in the if() statement. No way is that a a reason to delay the patch to the next CommitFest! Regards, Marti