Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On Fri, Sep 27, 2013 at 10:28 AM, Robert Haas robertmh...@gmail.com wrote: On Thu, Sep 26, 2013 at 5:18 PM, Robert Haas robertmh...@gmail.com wrote: On Thu, Sep 26, 2013 at 3:55 PM, David Rowley dgrowle...@gmail.com wrote: I think I must have forgot to save it before I emailed it... Here's the correct version. Ah ha. Looks better. I'm working on getting this committed now. Aside from some stylistic things, I've found one serious functional bug, which is that you need to test padding != 0, not padding 0, when deciding which path to take. No need to send a new patch, I've already fixed it in my local copy... Oops, negative padding. My bad. I was focusing too much on the benchmarks I think. Committed now. Let me know if I broke anything. Great, thanks for commiting! Thank you Albe for your review too! Regards David. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On Wed, Sep 25, 2013 at 4:46 AM, David Rowley dgrowle...@gmail.com wrote: Ok, I think I've managed to narrow the performance gap to just about nothing but noise, though to do this the code is now a bit bigger. I've added a series of tests to see if the padding is 0 and if it's not then I'm doing things the old way. I've also added a some code which does a fast test to see if it is worth while calling the padding processing function. This is just a simple if (*p = '9'), I'm not completely happy with that as it does look a bit weird, but to compensate I've added a good comment to explain what it is doing. Please find attached the new patch ... version v0.5 and also updated benchmark results. Are you sure this is the right set of benchmark results? This still reflects a 15-18% slowdown AFAICS. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On Fri, Sep 27, 2013 at 4:44 AM, Robert Haas robertmh...@gmail.com wrote: On Wed, Sep 25, 2013 at 4:46 AM, David Rowley dgrowle...@gmail.com wrote: Ok, I think I've managed to narrow the performance gap to just about nothing but noise, though to do this the code is now a bit bigger. I've added a series of tests to see if the padding is 0 and if it's not then I'm doing things the old way. I've also added a some code which does a fast test to see if it is worth while calling the padding processing function. This is just a simple if (*p = '9'), I'm not completely happy with that as it does look a bit weird, but to compensate I've added a good comment to explain what it is doing. Please find attached the new patch ... version v0.5 and also updated benchmark results. Are you sure this is the right set of benchmark results? This still reflects a 15-18% slowdown AFAICS. I think I must have forgot to save it before I emailed it... Here's the correct version. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company log_line_prefix_benchmark_stresslog_v0.5.xls Description: MS-Excel spreadsheet -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On Thu, Sep 26, 2013 at 3:55 PM, David Rowley dgrowle...@gmail.com wrote: I think I must have forgot to save it before I emailed it... Here's the correct version. Ah ha. Looks better. I'm working on getting this committed now. Aside from some stylistic things, I've found one serious functional bug, which is that you need to test padding != 0, not padding 0, when deciding which path to take. No need to send a new patch, I've already fixed it in my local copy... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On Thu, Sep 26, 2013 at 5:18 PM, Robert Haas robertmh...@gmail.com wrote: On Thu, Sep 26, 2013 at 3:55 PM, David Rowley dgrowle...@gmail.com wrote: I think I must have forgot to save it before I emailed it... Here's the correct version. Ah ha. Looks better. I'm working on getting this committed now. Aside from some stylistic things, I've found one serious functional bug, which is that you need to test padding != 0, not padding 0, when deciding which path to take. No need to send a new patch, I've already fixed it in my local copy... Committed now. Let me know if I broke anything. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On Wed, Sep 25, 2013 at 1:20 AM, Robert Haas robertmh...@gmail.com wrote: On Tue, Sep 24, 2013 at 5:04 AM, David Rowley dgrowle...@gmail.com wrote: So... I guess the question that I'd ask is, if you write a PL/pgsql function that does RAISE NOTICE in a loop a large number of times, can you measure any difference in how fast that function executes on the patch and unpatched code? If so, how much? I do see a 15-18% slow down with the patched version, so perhaps I'll need to look to see if I can speed it up a bit, although I do feel this benchmark is not quite a normal workload. Ouch! That's pretty painful. I agree that's not a normal workload, but I don't think it's an entirely unfair benchmark, either. There certainly are people who suffer because of the cost of logging as things are; for example, log_min_duration_statement is commonly used and can produce massive amounts of output on a busy system. I wouldn't mind too much if the slowdown you are seeing only occurred when the feature is actually used, but taking a 15-18% hit on logging even when the new formatting features aren't being used seems too expensive to me. Ok, I think I've managed to narrow the performance gap to just about nothing but noise, though to do this the code is now a bit bigger. I've added a series of tests to see if the padding is 0 and if it's not then I'm doing things the old way. I've also added a some code which does a fast test to see if it is worth while calling the padding processing function. This is just a simple if (*p = '9'), I'm not completely happy with that as it does look a bit weird, but to compensate I've added a good comment to explain what it is doing. Please find attached the new patch ... version v0.5 and also updated benchmark results. Regards David -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company log_line_prefix_benchmark_stresslog_v0.5.xls Description: MS-Excel spreadsheet log_line_formatting_v0.5.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On 9/25/13 4:46 AM, David Rowley wrote: Please find attached the new patch ... version v0.5 and also updated benchmark results. Please fix compiler warnings: elog.c: In function ‘log_line_prefix.isra.3’: elog.c:2436:22: warning: ‘padding’ may be used uninitialized in this function [-Wmaybe-uninitialized] elog.c:2172:6: note: ‘padding’ was declared here -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On Thu, Sep 26, 2013 at 4:57 AM, Peter Eisentraut pete...@gmx.net wrote: On 9/25/13 4:46 AM, David Rowley wrote: Please find attached the new patch ... version v0.5 and also updated benchmark results. Please fix compiler warnings: elog.c: In function ‘log_line_prefix.isra.3’: elog.c:2436:22: warning: ‘padding’ may be used uninitialized in this function [-Wmaybe-uninitialized] elog.c:2172:6: note: ‘padding’ was declared here Fixed in attached version. Regards David Rowley log_line_formatting_v0.6.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On Tue, Sep 24, 2013 at 5:16 AM, Robert Haas robertmh...@gmail.com wrote: On Fri, Sep 20, 2013 at 11:28 PM, David Rowley dgrowle...@gmail.com wrote:\ I put the above results into the attached spreadsheet. On my intel i5 laptop I'm seeing a slow down of about 1 second per million queries for the longer log_line_prefix and about 1 second per 5 million queries with the shorter log_line_prefix. In the attached spreadsheet I've mocked up some very rough estimates on the performance cost of this change. I think a while ago I read about a benchmark Robert ran on a 64 core machine which ran around 400k transactions per second. I've included some workings in the spreadsheet to show how this change would affect that benchmark, but I have assumed that the hardware would only be able to execute the log_line_prefix function at the same speed as my i5 laptop. With this very worst case estimates my calculations say that the performance hit is 0.6% with the log_line_prefix which contains all of the variables and 0.11% for the shorter log_line_prefix. I would imagine that the hardware that performed 400k TPS would be able to run log_line_prefix faster than my 3 year old i5 laptop, so this is likely quite a big over estimation of the hit. Well, on those tests, I was hardly logging anything, so it's hard to really draw any conclusions that way. I think the real concern with this patch, performance-wise, is what happens in environments where there is a lot of logging going on. We've had previous reports of people who can't even enable the logging they want because the performance cost is unacceptably high. That's why we added that logging hook in 9.2 or 9.3, so that people can write alternative loggers that just throw away messages if the recipient can't eat them fast enough. I wouldn't be keen to accept a 25% performance hit on logging overall, but log_line_prefix() is only a small part of that cost. So... I guess the question that I'd ask is, if you write a PL/pgsql function that does RAISE NOTICE in a loop a large number of times, can you measure any difference in how fast that function executes on the patch and unpatched code? If so, how much? Ok, I've been doing a bit of benchmarking on this. To mock up a really fast I/O system I created a RAM disk which I will ask Postgres to send the log files to. mkdir /tmp/ramdisk; chmod 777 /tmp/ramdisk mount -t tmpfs -o size=256M tmpfs /tmp/ramdisk/ I created the following function in plpgsql create function stresslog(n int) returns int as $$ begin while n 0 loop raise notice '%', n; n := n - 1; end loop; return 0; end; $$ language plpgsql; I was running postgreql with david@ubuntu64:~/9.4/bin$ ./pg_ctl start -D /home/david/9.4/data -l /tmp/ramdisk/pg.log I ran the following command 5 times for each version. client_min_message is set to warning and log_min_message is set to notice postgres=# select stresslog(10); stresslog --- 0 (1 row) I do see a 15-18% slow down with the patched version, so perhaps I'll need to look to see if I can speed it up a bit, although I do feel this benchmark is not quite a normal workload. Please see below the results, or if you want to play about with them a bit, please use the attached spreadsheet. * Round 1 Patched: log_line_prefix = %a %u %d %r %h %p %t %m %i %e %c %l %s %v %x Time:1822.105ms Time:1762.529ms Time:1839.724ms Time:1837.372ms Time:1813.240ms unpatched: log_line_prefix = %a %u %d %r %h %p %t %m %i %e %c %l %s %v %x Time:1519.032ms Time:1528.760ms Time:1484.074ms Time:1552.786ms Time:1569.410ms * Round 2 patched: log_line_prefix = %a %u %d %e Time:625.969ms Time:668.444ms Time:648.310ms Time:663.655ms Time:715.397ms unpatched: log_line_prefix = %a %u %d %e Time:598.146ms Time:518.827ms Time:532.858ms Time:529.584ms Time:537.276ms Regards David Rowley log_line_prefix_benchmark_stresslog_v0.4.xls Description: MS-Excel spreadsheet -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On Tue, Sep 24, 2013 at 5:04 AM, David Rowley dgrowle...@gmail.com wrote: So... I guess the question that I'd ask is, if you write a PL/pgsql function that does RAISE NOTICE in a loop a large number of times, can you measure any difference in how fast that function executes on the patch and unpatched code? If so, how much? I do see a 15-18% slow down with the patched version, so perhaps I'll need to look to see if I can speed it up a bit, although I do feel this benchmark is not quite a normal workload. Ouch! That's pretty painful. I agree that's not a normal workload, but I don't think it's an entirely unfair benchmark, either. There certainly are people who suffer because of the cost of logging as things are; for example, log_min_duration_statement is commonly used and can produce massive amounts of output on a busy system. I wouldn't mind too much if the slowdown you are seeing only occurred when the feature is actually used, but taking a 15-18% hit on logging even when the new formatting features aren't being used seems too expensive to me. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
David Rowley escribió: I do see a 15-18% slow down with the patched version, so perhaps I'll need to look to see if I can speed it up a bit, although I do feel this benchmark is not quite a normal workload. Ouch. That's certainly way too much. Is the compiler inlining process_log_prefix_padding()? If not, does it do it if you add inline to it? That might speed up things a bit. If that's not enough, maybe you need some way to return to the original coding for the case where no padding is set in front of each option. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On 2013-09-24 13:51:04 -0300, Alvaro Herrera wrote: David Rowley escribió: I do see a 15-18% slow down with the patched version, so perhaps I'll need to look to see if I can speed it up a bit, although I do feel this benchmark is not quite a normal workload. Ouch. That's certainly way too much. Is the compiler inlining process_log_prefix_padding()? If not, does it do it if you add inline to it? That might speed up things a bit. If that's not enough, maybe you need some way to return to the original coding for the case where no padding is set in front of each option. From a very short look without actually running it I'd guess the issue is all the $* things you're now passing to do appendStringInfo (which passes them off to vsnprintf). How does it look without that? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On 2013-09-24 19:56:32 +0200, Andres Freund wrote: On 2013-09-24 13:51:04 -0300, Alvaro Herrera wrote: David Rowley escribió: I do see a 15-18% slow down with the patched version, so perhaps I'll need to look to see if I can speed it up a bit, although I do feel this benchmark is not quite a normal workload. Ouch. That's certainly way too much. Is the compiler inlining process_log_prefix_padding()? If not, does it do it if you add inline to it? That might speed up things a bit. If that's not enough, maybe you need some way to return to the original coding for the case where no padding is set in front of each option. From a very short look without actually running it I'd guess the issue is all the $* things you're now passing to do appendStringInfo (which passes them off to vsnprintf). How does it look without that? That's maybe misunderstandable, what I mean is to have an if (padding 0) around the the changed appendStringInfo invocations and use the old ones otherwise. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On Wed, Sep 25, 2013 at 6:25 AM, Andres Freund and...@2ndquadrant.comwrote: On 2013-09-24 19:56:32 +0200, Andres Freund wrote: On 2013-09-24 13:51:04 -0300, Alvaro Herrera wrote: David Rowley escribió: I do see a 15-18% slow down with the patched version, so perhaps I'll need to look to see if I can speed it up a bit, although I do feel this benchmark is not quite a normal workload. Ouch. That's certainly way too much. Is the compiler inlining process_log_prefix_padding()? If not, does it do it if you add inline to it? That might speed up things a bit. If that's not enough, maybe you need some way to return to the original coding for the case where no padding is set in front of each option. From a very short look without actually running it I'd guess the issue is all the $* things you're now passing to do appendStringInfo (which passes them off to vsnprintf). How does it look without that? That's maybe misunderstandable, what I mean is to have an if (padding 0) around the the changed appendStringInfo invocations and use the old ones otherwise. Yeah I had the same idea to try that next. I suspect that's where the slow down is rather than the processing of the padding. I'm thinking these small tweaks are going to make the code a bit ugly, but I agree about the 15-18% slowdown is a no go. The only other thing apart from checking if padding 0 is to check if the char after the % is '9', in that case it can't be formatting as we're only allowing '-' and '0' to '9'. Although I think that's a bit hackish, but perhaps it is acceptable if it helps narrow the performance gap. More benchmarks to follow soon. David Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On Fri, Sep 20, 2013 at 11:28 PM, David Rowley dgrowle...@gmail.com wrote:\ I put the above results into the attached spreadsheet. On my intel i5 laptop I'm seeing a slow down of about 1 second per million queries for the longer log_line_prefix and about 1 second per 5 million queries with the shorter log_line_prefix. In the attached spreadsheet I've mocked up some very rough estimates on the performance cost of this change. I think a while ago I read about a benchmark Robert ran on a 64 core machine which ran around 400k transactions per second. I've included some workings in the spreadsheet to show how this change would affect that benchmark, but I have assumed that the hardware would only be able to execute the log_line_prefix function at the same speed as my i5 laptop. With this very worst case estimates my calculations say that the performance hit is 0.6% with the log_line_prefix which contains all of the variables and 0.11% for the shorter log_line_prefix. I would imagine that the hardware that performed 400k TPS would be able to run log_line_prefix faster than my 3 year old i5 laptop, so this is likely quite a big over estimation of the hit. Well, on those tests, I was hardly logging anything, so it's hard to really draw any conclusions that way. I think the real concern with this patch, performance-wise, is what happens in environments where there is a lot of logging going on. We've had previous reports of people who can't even enable the logging they want because the performance cost is unacceptably high. That's why we added that logging hook in 9.2 or 9.3, so that people can write alternative loggers that just throw away messages if the recipient can't eat them fast enough. I wouldn't be keen to accept a 25% performance hit on logging overall, but log_line_prefix() is only a small part of that cost. So... I guess the question that I'd ask is, if you write a PL/pgsql function that does RAISE NOTICE in a loop a large number of times, can you measure any difference in how fast that function executes on the patch and unpatched code? If so, how much? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On Fri, Sep 20, 2013 at 12:48 AM, Peter Eisentraut pete...@gmx.net wrote: Something is weird in your latest patch. The header is: diff -u -r b/postgresql/doc/src/sgml/config.sgml a/postgresql/doc/src/sgml/config.sgml --- b/postgresql/doc/src/sgml/config.sgml 2013-09-09 23:40:52.356371191 +1200 +++ a/postgresql/doc/src/sgml/config.sgml 2013-09-19 22:13:26.236681468 +1200 This doesn't apply with patch -p1 or -p0. (You need -p2.) Your previous patch in this thread seemed OK. You might want to check what you did there. I moved the source around and I've patched against it again. New patch attached. David log_line_formatting_v0.3.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
David Rowley wrote: I moved the source around and I've patched against it again. New patch attached. Thank you, marked as ready for committer. Yours, Laurenz Albe -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On Fri, Sep 20, 2013 at 3:15 AM, Albe Laurenz laurenz.a...@wien.gv.at wrote: David Rowley wrote: I moved the source around and I've patched against it again. New patch attached. Thank you, marked as ready for committer. /* + * helper function for processing the format string in + * log_line_prefix() + * This function returns NULL if it finds something which + * it deems invalid for the log_line_prefix string. + */ Comments should be formatted as a single paragraph. If you want multiple paragraphs, you need to include a line that's blank except for the leading *. +static const char +*process_log_prefix_padding(const char *p, int *ppadding) The asterisk should be on the previous line, separated from char by a space. + appendStringInfo(buf, %*ld, padding, log_line_number); Is %* supported by all versions of printf() on every platform we support? Earlier there was some discussion of performance. Was that tested? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
On Sat, Sep 21, 2013 at 7:18 AM, Robert Haas robertmh...@gmail.com wrote:
On Fri, Sep 20, 2013 at 3:15 AM, Albe Laurenz laurenz.a...@wien.gv.at
wrote:
David Rowley wrote:
I moved the source around and I've patched against it again. New patch
attached.
Thank you, marked as ready for committer.
/*
+ * helper function for processing the format string in
+ * log_line_prefix()
+ * This function returns NULL if it finds something which
+ * it deems invalid for the log_line_prefix string.
+ */
Comments should be formatted as a single paragraph. If you want
multiple paragraphs, you need to include a line that's blank except
for the leading *.
+static const char
+*process_log_prefix_padding(const char *p, int *ppadding)
The asterisk should be on the previous line, separated from char by a
space.
I've attached another revision of the patch which cleans up the comment for
the process_log_prefix_padding function to be more like the comments
earlier in the same file. I have also moved the asterisk to the previous
line.
+ appendStringInfo(buf, %*ld, padding,
log_line_number);
Is %* supported by all versions of printf() on every platform we support?
Do you specifically mean %*ld, or %* in general? As I can see various other
places where %*s is used in the source by looking at grep -r %\* .
But if you do mean specifically %*ld, then we did already use %ld here and
since there are places which use %*s, would it be wrong to assume that %*ld
is ok?
Earlier there was some discussion of performance. Was that tested?
I've done some performance tests now. I assume that the processing of the
log line prefix would be drowned out by any testing of number of
transactions per second, so I put a few lines at the start of
send_message_to_server_log():
Which ended up looking like:
static void
send_message_to_server_log(ErrorData *edata)
{
StringInfoData buf;
int i;
float startTime, endTime;
startTime = (float)clock()/CLOCKS_PER_SEC;
StringInfoData tmpbuf;
for (i = 0; i 100; i++)
{
initStringInfo(tmpbuf);
log_line_prefix(tmpbuf, edata);
pfree(tmpbuf.data);
}
endTime = (float)clock()/CLOCKS_PER_SEC;
printf(log_line_prefix (%s) in %f seconds\n, Log_line_prefix, endTime
- startTime);
initStringInfo(buf);
...
I am seeing a slow down in the processing of the 2 log_line_prefix strings
that I tested with. Here are the results:
Patched (%a %u %d %p %t %m %i %e %c %l %s %v %x )
david@ubuntu64:~/9.4/bin$ ./postgres -D /home/david/9.4/data/
log_line_prefix (%a %u %d %p %t %m %i %e %c %l %s %v %x ) in 3.93
seconds
62324 2013-09-20 05:37:30 NZST 2013-09-20 05:37:30.455 NZST 0
523b3656.f374 101 2013-09-20 05:37:26 NZST 0 LOG: database system was
shut down at 2013-09-20 05:36:21 NZST
log_line_prefix (%a %u %d %p %t %m %i %e %c %l %s %v %x ) in 3.94
seconds
62329 2013-09-20 05:37:38 NZST 2013-09-20 05:37:38.724 NZST 0
523b365a.f379 101 2013-09-20 05:37:30 NZST 0 LOG: autovacuum launcher
started
log_line_prefix (%a %u %d %p %t %m %i %e %c %l %s %v %x ) in 3.96
seconds
62323 2013-09-20 05:37:38 NZST 2013-09-20 05:37:38.756 NZST 0
523b3656.f373 101 2013-09-20 05:37:26 NZST 0 LOG: database system is
ready to accept connections
log_line_prefix (%a %u %d %p %t %m %i %e %c %l %s %v %x ) in 4.82
seconds
psql david postgres 62331 2013-09-20 05:38:43 NZST 2013-09-20 05:38:43.490
NZST SELECT 22012 523b3688.f37b 101 2013-09-20 05:38:16 NZST 2/4 0
ERROR: division by zero
psql david postgres 62331 2013-09-20 05:38:43 NZST 2013-09-20 05:38:43.490
NZST SELECT 22012 523b3688.f37b 102 2013-09-20 05:38:16 NZST 2/4 0
STATEMENT: select 1/0;
log_line_prefix (%a %u %d %p %t %m %i %e %c %l %s %v %x ) in 4.69
seconds
psql david postgres 62331 2013-09-20 05:39:35 NZST 2013-09-20 05:39:35.900
NZST SELECT 22012 523b3688.f37b 203 2013-09-20 05:38:16 NZST 2/5 0
ERROR: division by zero
psql david postgres 62331 2013-09-20 05:39:35 NZST 2013-09-20 05:39:35.900
NZST SELECT 22012 523b3688.f37b 204 2013-09-20 05:38:16 NZST 2/5 0
STATEMENT: select 1/0;
Unpatched (%a %u %d %p %t %m %i %e %c %l %s %v %x )
david@ubuntu64:~/9.4/bin$ ./postgres -D /home/david/9.4/data/
log_line_prefix (%a %u %d %p %t %m %i %e %c %l %s %v %x ) in 3.12
seconds
925 2013-09-20 05:45:48 NZST 2013-09-20 05:45:48.483 NZST 0
523b3849.39d 101 2013-09-20 05:45:45 NZST 0 LOG: database system was
shut down at 2013-09-20 05:40:47 NZST
log_line_prefix (%a %u %d %p %t %m %i %e %c %l %s %v %x ) in 3.10
seconds
924 2013-09-20 05:45:54 NZST 2013-09-20 05:45:54.970 NZST 0
523b3849.39c 101 2013-09-20 05:45:45 NZST 0 LOG: database system is
ready to accept connections
log_line_prefix (%a %u %d %p %t %m %i %e %c %l %s %v %x ) in 3.12
seconds
931 2013-09-20 05:45:55 NZST 2013-09-20 05:45:55.015 NZST 0
523b384c.3a3 101 2013-09-20
[HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
(Forgot to copy list) From: David Rowley [mailto:dgrowle...@gmail.com] Sent: 19 September 2013 22:35 To: Albe Laurenz Subject: Re: REVIEW: Allow formatting in log_line_prefix Hi Laurenz, Thanks for the review. Please see my comments below and the updated patch attached. On Thu, Sep 19, 2013 at 2:18 AM, Albe Laurenz laurenz.a...@wien.gv.at mailto:laurenz.a...@wien.gv.at wrote: This is a review for patch caaphdvpagtypzb2kwa0mmtksayg9+vashyjmjfatngxr1ad...@mail.gmail.com mailto:caaphdvpagtypzb2kwa0mmtksayg9%2bvashyjmjfatngxr1ad...@mail.gmail.com The patch is readable, applies fine and builds without warnings. It contains sufficient documentation. It works as it should, no crashes or errors. It is well written, in fact it improves the readability of the log_line_prefix function in backend/utils/error/elog.c. I think that this is a useful feature as it can help to make stderr logging more readable for humans. I have a few gripes with the English: - In the documentation patch: + numeric literal after the % and before the option. A negative + padding will cause the status information to be padded on the + right with spaces to give it a minimum width. Whereas a positive + padding will pad on the left. Padding can be useful keep log + files more human readable. I think that there should be a comma, not a period, before whereas. I've made the change you request here and also made a change to my last sentence, which hopefully is an improvement. - The description for the function process_log_prefix_padding should probably mention that it returns NULL if it encounters bad syntax. I've added a comment before the function for this. - This comment: + /* Format is invalid if it ends with the padding number */ should begin with lower case. Fixed, but not quite sure of the reason for lack of caps at the moment. - In this comment: + /* +* Process any formatting which may exist after the '%'. +* Note that this moves p passed the padding number +* if it exists. +*/ It should be past, not passed. Fixed. If these details get fixed, I'll mark the patch ready for committer. Yours, Laurenz Albe log_line_formatting_v0.2.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] FW: REVIEW: Allow formatting in log_line_prefix
Something is weird in your latest patch. The header is: diff -u -r b/postgresql/doc/src/sgml/config.sgml a/postgresql/doc/src/sgml/config.sgml --- b/postgresql/doc/src/sgml/config.sgml 2013-09-09 23:40:52.356371191 +1200 +++ a/postgresql/doc/src/sgml/config.sgml 2013-09-19 22:13:26.236681468 +1200 This doesn't apply with patch -p1 or -p0. (You need -p2.) Your previous patch in this thread seemed OK. You might want to check what you did there. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
