Re: [PATCHES] Proposed patch - psql wraps at window width
Alvaro Herrera wrote: * Don't lose warning comments like this one (unless you've removed the assumption of course) /* * Assumption: This code used only on strings * without multibyte characters, otherwise * this_line-width strlen(this_ptr) and we get * an overflow */ In fact, that particular assumption was causing a problem, causing a segfault. I can't be certain, because the multibyte stuff is pretty intense, but I think I nailed it. Thanks for all your comments! -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
[PATCHES] Coding standards
Alvaro Herrera wrote:
People [are] complaining here that we don't teach people here anyway, so
hopefully my comments were still useful :-)
Yes they are useful. As a new patcher, where should I look for coding
standards? How about a little FAQ at the
top of the CVS source tree?
Though, darn it, I sure like //
And my vi is set to:
set sw=4
set ts=4
set expandtab
Because my corporate projects require spaces not tabs.
Some random comments:
* Don't use C++ style comments (//). Some compilers don't like these.
* Beware of brace position: we use braces on their own, indented at the
start of a new line, so
! while(--count) {
! lines++;
! lines-ptr = NULL;
! lines-width = 0;
! }
becomes
! while(--count)
! {
! lines++;
! lines-ptr = NULL;
! lines-width = 0;
! }
(with correct indentation anyway)
* Always use tabs, not spaces, to indent. Tabs are 4 spaces wide.
* Don't use double stars in comments.
* } else is forbidden too. Use two separate lines.
--
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Peter Eisentraut wrote: Bruce Momjian wrote: I checked the use of COLUMNS and it seems bash updates the environment variable when a window is resized. I added ioctl(TIOCGWINSZ) if COLUMNS isn't set. We already had a call in print.c for detecting the number of rows on the screen to determine if the pager should be used. Seems COLUMNS should take precedence over ioctl(), right? Considering that the code to determine the row count is undisputed so far, the column count detection should work the same. That is, we might not need to look at COLUMNS at all. Unless there is a use case for overriding the column count (instead of just turning off the wrapping). I asked the folks over at Experts Exchange to test the behavior of the ioctl and $COLUMNS on various platforms. I'd been told that I would face huge problems if a console was resized. But the results were pretty consistent, and nothing had problems with resize: http://www.experts-exchange.com/Programming/Open_Source/Q_23243646.html It appears impossible to override $COLUMNS, on some platforms as the readline call sets it. On many platforms $COLUMNS is null until the call to readline. OSX does not set $COLUMNS at all. In short, I recommend the ioctl instead. In order to provide a way to wrap output to a pipe, I think a different mechanism will have to be found. -Bryce -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Coding standards
Bryce Nesbitt wrote: Alvaro Herrera wrote: People [are] complaining here that we don't teach people here anyway, so hopefully my comments were still useful :-) Yes they are useful. As a new patcher, where should I look for coding standards? How about a little FAQ at the top of the CVS source tree? Though, darn it, I sure like // And my vi is set to: set sw=4 set ts=4 set expandtab Because my corporate projects require spaces not tabs. See the developer FAQ on the website. IIRC, it even contains what you should put in your vi config file to make it do the right thing... //Magnus -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Tom Lane [EMAIL PROTECTED] writes: Bryce Nesbitt [EMAIL PROTECTED] writes: pre wrap=I checked the use of COLUMNS and it seems bash updates the environment variable when a window is resized. [ Please get rid of the HTML formatting ... ] Bash can update the environment all it wants, but that will not affect what is seen by a program that's already running. Personally I often resize the window while psql is running, and I expect that to work. Hm, then having COLUMNS override the ioctl isn't such a good idea. Checking GNU ls source the ioctl overrides COLUMNS if it works, but there's a --width option which trumps both of the other two. I guess just a psql variable would be the equivalent. I'm with Peter on this one: we have used ioctl, and nothing else, to determine the vertical window dimension for many years now, to the tune of approximately zero complaints. It's going to take one hell of a strong argument to persuade me that determination of the horizontal dimension should not work exactly the same way. Well the cases are not analogous. Firstly, the window height doesn't actually alter the output. Secondly there's really no downside in a false positive since most pagers just exit if they decide the output fit on the screen -- which probably explains why no ssh users have complained... And in any case you can always override it by piping the output to a pager yourself -- which is effectively all I'm suggesting doing here. So here are your two hella-strong arguments: a) not all terminals support the ioctl. Emacs shell users may be eccentric but surely using psql over ssh isn't especially uncommon. Falling back to COLUMNS is standard, GNU ls is not alone, Solaris and FreeBSD both document supporting COLUMNS. b) you don't necessarily *want* the output formatted to fill the screen. You may be generating a report to email and want to set the width to the RFC recommended 72 characters. You may just have a full screen terminal but not enjoy reading 200-character long lines -- try it, it's really hard: MANWIDTH If $MANWIDTH is set, its value is used as the line length for which manual pages should be formatted. If it is not set, manual pages will be formatted with a line length appropriate to the current terminal (using an ioctl(2) if available, the value of $COLUMNS, or falling back to 80 characters if neither is available). Cat pages will only be saved when the default formatting can be used, that is when the terminal line length is between 66 and 80 characters. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's On-Demand Production Tuning -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Peter Eisentraut wrote: Bruce Momjian wrote: I checked the use of COLUMNS and it seems bash updates the environment variable when a window is resized. I added ioctl(TIOCGWINSZ) if COLUMNS isn't set. We already had a call in print.c for detecting the number of rows on the screen to determine if the pager should be used. Seems COLUMNS should take precedence over ioctl(), right? Considering that the code to determine the row count is undisputed so far, the column count detection should work the same. That is, we might not need to look at COLUMNS at all. Unless there is a use case for overriding the column count (instead of just turning off the wrapping). I asked the folks over at Experts Exchange to test the behavior of the ioctl and $COLUMNS on various platforms. I'd been told that I would face huge problems if a console was resized. But the results were pretty consistent, and $COLUMNS had no problems with resize: http://www.experts-exchange.com/Programming/Open_Source/Q_23243646.html But appears impossible to override $COLUMNS, on some platforms as the readline call sets it. On many platforms $COLUMNS is null until the call to readline. OSX does not set $COLUMNS at all. So I think the ioctl should be used to determine the wrap width for terminals, and some other mechanism used for pipes. There's no way I'd want wrapping as the default for pipe output. I was not bold enough to propose that wrapping be the default behavior for the terminal. -Bryce -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
Bryce Nesbitt [EMAIL PROTECTED] writes: I asked the folks over at Experts Exchange to test the behavior of the ioctl I always thought that was a joke domain name, like Pen Island.com. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's PostGIS support! -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Coding standards
Bryce Nesbitt wrote: Alvaro Herrera wrote: People [are] complaining here that we don't teach people here anyway, so hopefully my comments were still useful :-) Yes they are useful. As a new patcher, where should I look for coding standards? How about a little FAQ at the top of the CVS source tree? The developer's FAQ is supposed to contain this kind of thing, but I think it's rather thin on actual details. (Some time ago it was proposed to create a style guide, but people here thought it was a waste of time and it will not cover what's really important, so we're stuck with repeating the advice over and over.) Though, darn it, I sure like // And my vi is set to: set sw=4 set ts=4 set expandtab Because my corporate projects require spaces not tabs. I use this: :if match(getcwd(), /home/alvherre/Code/CVS/pgsql) == 0 : set cinoptions=(0 : set tabstop=4 : set shiftwidth=4 : let $CSCOPE_DB=substitute(getcwd(), ^\\(.*/pgsql/source/[^/]*\\)/.*, \\1, ) : let tags=substitute(getcwd(), ^\\(.*/pgsql/source/[^/]*\\)/.*, \\1, ) . /tags : let path=substitute(getcwd(), ^\\(.*/pgsql/source/[^/]*\\)/.*, \\1, ) . /src/include :endif Of course, you need to adjust the paths. The cscope, tags and path things let me quickly navigate through the files, but they don't affect the editor behavior. I never set expandtab so it's not a problem for me, but I guess you can do :set noexpandtab in that block to reset it for Postgres use. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Proposed patch - psql wraps at window width
On Fri, 18 Apr 2008 17:21:26 +0100 Gregory Stark [EMAIL PROTECTED] wrote: Bryce Nesbitt [EMAIL PROTECTED] writes: I asked the folks over at Experts Exchange to test the behavior of the ioctl I always thought that was a joke domain name, like Pen Island.com. Its not and a lot of postgresql users interact there. Joshua D. Drake -- The PostgreSQL Company since 1997: http://www.commandprompt.com/ PostgreSQL Community Conference: http://www.postgresqlconference.org/ United States PostgreSQL Association: http://www.postgresql.us/ Donate to the PostgreSQL Project: http://www.postgresql.org/about/donate -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Coding standards
[EMAIL PROTECTED] (Bryce Nesbitt) writes: Alvaro Herrera wrote: People [are] complaining here that we don't teach people here anyway, so hopefully my comments were still useful :-) Yes they are useful. As a new patcher, where should I look for coding standards? How about a little FAQ at the top of the CVS source tree? Though, darn it, I sure like // And my vi is set to: set sw=4 set ts=4 set expandtab Because my corporate projects require spaces not tabs. Note that you can find config for vim and emacs to get them to support the coding standards in: /opt/src/pgsql-HEAD/src/tools/editors/emacs.samples /opt/src/pgsql-HEAD/src/tools/editors/vim.samples For vim, the essentials are thus: :if match(getcwd(), /pgsql) =0 || match(getcwd(), /postgresql) = 0 : set cinoptions=(0 : set tabstop=4 : set shiftwidth=4 :endif The hooks are slightly different (though not by spectacularly much, somewhat surprisingly) for Emacs... -- let name=cbbrowne and tld=cbbrowne.com in name ^ @ ^ tld;; http://cbbrowne.com/info/advocacy.html A language that doesn't affect the way you think about programming, is not worth knowing. -- Alan J. Perlis -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
[PATCHES] Testing pg_terminate_backend()
bruce wrote: Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: Tom Lane wrote: The closest thing I can think of to an automated test is to run repeated sets of the parallel regression tests, and each time SIGTERM a randomly chosen backend at a randomly chosen time. Then see if anything funny Yep, that was my plan, plus running the parallel regression tests you get the possibility of 2 backends. I was intentionally suggesting only one kill per test cycle. Multiple kills will probably create an O(N^2) explosion in the set of possible downstream-failure deltas. I doubt you'd really get any improvement in testing coverage to justify the much larger amount of hand validation needed. It also strikes me that you could make some simple alterations to the regression tests to reduce the set of observable downstream deltas. For example, anyplace where a test loads a table with successive INSERTs and that table is used by later tests, wrap the INSERT sequence with BEGIN/END. Then there is only one possible downstream delta (empty table) and not N different possibilities for an N-row table. I have added pg_terminate_backend() to use SIGTERM and will start running tests as discussed with Tom. I will post my scripts too. Attached is my test script. I ran it for 14 hours (asserts on), running 450 regression tests, with up to seven backends killed per regression test. I have processed the combined regression.diffs files by pickouting out all the new error messages. I don't see anything unusual in there. Should I run it differently? -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + #!/bin/bash REGRESSION_DURATION=80 # average duration of regression test in seconds OUTFILE=/rtmp/regression.sigterm # To analyze output, use: # grep '^\+ *[A-Z][A-Z]*:' /rtmp/regression.sigterm | sort | uniq | less cd /pg/test/regress while : do ( SLEEP=`expr $RANDOM \* $REGRESSION_DURATION / 32767` echo Sleeping $SLEEP seconds sleep $SLEEP echo Trying kill # send up to 7 kill signals for X in 1 2 3 4 5 6 7 do psql -p 55432 -qt -c SELECT pg_terminate_backend(stat.procpid) FROM (SELECT procpid FROM pg_stat_activity ORDER BY random() LIMIT 1) AS stat template1 2 /dev/null if [ $? -eq 0 ] thenecho Kill sent fi sleep 5 done ) gmake check wait [ -s regression.diffs ] cat regression.diffs $OUTFILE done + CONTEXT: COPY bool_test, line 1: TRUE null FALSE null + CONTEXT: COPY create_table_test, line 1: 5 10 + CONTEXT: COPY x, line 0: + CONTEXT: COPY x, line 1: 1 test_1 + CONTEXT: COPY x, line 1: 4000:\X:C:\X:\X + CONTEXT: SQL function declares_cursor + CONTEXT: SQL function max_xacttest + ERROR: INSERT has more expressions than target columns + ERROR: cannot drop table test1 because other objects depend on it + ERROR: column a of relation xacttest does not exist + ERROR: column f1 does not exist + ERROR: column fooid does not exist + ERROR: current transaction is aborted, commands ignored until end of transaction block + ERROR: cursor foo25 does not exist + ERROR: function getfoo(integer) does not exist + ERROR: index onek_nulltest does not exist + ERROR: index wowidx does not exist + ERROR: no such savepoint + ERROR: prepared statement q5 does not exist + ERROR: relation a_star does not exist + ERROR: relation aggtest does not exist + ERROR: relation array_index_op_test does not exist + ERROR: relation array_op_test does not exist + ERROR: relation b_star does not exist + ERROR: relation bt_f8_heap does not exist + ERROR: relation bt_i4_heap does not exist + ERROR: relation bt_name_heap does not exist + ERROR: relation bt_txt_heap does not exist + ERROR: relation c_star does not exist + ERROR: relation d_star does not exist + ERROR: relation e_star does not exist + ERROR: relation emp does not exist + ERROR: relation equipment_r does not exist + ERROR: relation f_star does not exist + ERROR: relation fast_emp4000 does not exist + ERROR: relation foo already exists + ERROR: relation gcircle_tbl does not exist + ERROR: relation gpolygon_tbl does not exist + ERROR: relation hash_f8_heap does not exist + ERROR: relation hash_i4_heap does not exist + ERROR: relation hash_name_heap does not exist + ERROR: relation hash_txt_heap does not exist + ERROR: relation hobbies_r does not exist + ERROR: relation ihighway does not exist + ERROR: relation int2_tbl does not exist + ERROR:
Re: [PATCHES] Testing pg_terminate_backend()
Bruce Momjian [EMAIL PROTECTED] writes: Attached is my test script. I ran it for 14 hours (asserts on), running 450 regression tests, with up to seven backends killed per regression test. Hmm, there are something on the order of 1 SQL commands in our regression tests, so even assuming perfect randomness you've exercised SIGTERM on maybe 10% of them --- and of course there's multiple places in a complex DDL command where SIGTERM might conceivably be a problem. Who was volunteering to run this 24x7 for awhile? SLEEP=`expr $RANDOM \* $REGRESSION_DURATION / 32767` Uh, where's the randomness coming from? regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] float4/float8/int64 passed by value with tsearch fixup
Zoltan Boszormenyi wrote: - float4 conversion is risk free (patch #1) I applied this #1 patch. It needed some further adjustments; in particular contrib/btree_gist regression check was crashing, and utils/fmgr/README needed updating. With contrib/seg also adjusted to use float4 instead of float32, and thus the last usage of float32 gone, I am now wondering if it would be a good idea to remove the float32 and float32data definitions in c.h. Thanks to Zoltan for the patch. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] float4/float8/int64 passed by value with tsearch fixup
Alvaro Herrera wrote: With contrib/seg also adjusted to use float4 instead of float32, and thus the last usage of float32 gone, I am now wondering if it would be a good idea to remove the float32 and float32data definitions in c.h. Ok, the buildfarm is going yellow over this change. On cardinal I see this: *** ./expected/seg.out Fri Apr 18 20:45:38 2008 --- ./results/seg.out Fri Apr 18 20:59:40 2008 *** *** 1069,1218 SELECT seg_lower(s), seg_center(s), seg_upper(s) FROM test_seg WHERE s @ '11.2..11.3' OR s IS NULL ORDER BY s; seg_lower | seg_center | seg_upper ! ---++--- ! -Infinity | -Infinity |40 ! -Infinity | -Infinity |82 ! -Infinity | -Infinity |90 ! 1 | 7 |13 !1.3 | 6.65 |12 ! 2 | 6.75 | 11.5 !2.1 | 6.95 | 11.8 !2.3 | Infinity | Infinity !2.3 | Infinity | Infinity !2.4 | 6.85 | 11.3 !2.5 | 7 | 11.5 !2.5 | 7.15 | 11.8 !2.6 | Infinity | Infinity !2.7 | 7.35 |12 ! 3 | Infinity | Infinity ! 3 | 30.5 |58 !3.1 |7.3 | 11.5 !3.5 |7.5 | 11.5 !3.5 | 7.85 | 12.2 ! 4 | 8 |12 ! 4 | Infinity | Infinity ! 4 | 8 |12 ! 4 | 7.85 | 11.7 ! 4 | 8.25 | 12.5 ! 4 |8.5 |13 ! 4 | 32 |60 ! 4 | Infinity | Infinity !4.2 | 7.85 | 11.5 !4.2 | 7.95 | 11.7 !4.5 | 8.25 |12 !4.5 | 8 | 11.5 !4.5 | 8.25 |12 !4.5 | 8.25 |12 !4.5 |8.5 | 12.5 !4.5 | 59.75 | 115 !4.7 | 8.25 | 11.8 !4.8 | 8.15 | 11.5 !4.8 |8.2 | 11.6 !4.8 | 8.65 | 12.5 !4.8 | Infinity | Infinity !4.9 | 8.45 |12 !4.9 | Infinity | Infinity ! 5 | 8.25 | 11.5 ! 5 |8.5 |12 ! 5 | 17.5 |30 ! 5 |8.2 | 11.4 ! 5 | 8.25 | 11.5 ! 5 |8.3 | 11.6 ! 5 | 8.35 | 11.7 ! 5 |8.5 |12 ! 5 |8.5 |12 ! 5 |8.5 |12 !5.2 | 8.35 | 11.5 !5.2 |8.6 |12 ! 5.25 | 8.625 |12 !5.3 |8.4 | 11.5 !5.3 | 9.15 |13 !5.3 | 47.65 |90 !5.3 | Infinity | Infinity !5.4 | Infinity | Infinity !5.5 |8.5 | 11.5 !5.5 |8.6 | 11.7 !5.5 | 8.75 |12 !5.5 | 8.75 |12 !5.5 | 9 | 12.5 !5.5 |9.5 | 13.5 !5.5 | Infinity | Infinity !5.5 | Infinity | Infinity !5.7 | Infinity | Infinity !5.9 | Infinity | Infinity ! 6 | 8.75 | 11.5 ! 6 | 9 |12 ! 6 | 8.75 | 11.5 ! 6 |9.5 |13 ! 6 | 8.75 | 11.5 !6.1 | 9.05 |12 !6.1 | Infinity | Infinity !6.2 | 8.85 | 11.5 !6.3 | Infinity | Infinity !6.5 | 9 | 11.5 !6.5 | 9.25 |12 !6.5 | 9.25 |12 !6.5 | Infinity | Infinity !6.6 | Infinity | Infinity !6.7 |9.1 | 11.5 !6.7 | Infinity | Infinity ! 6.75 | Infinity | Infinity !6.8 | Infinity | Infinity !6.9 | 9.55 | 12.2 !6.9 | 48.45 |90 !6.9 | Infinity | Infinity ! 7 | 9.25 | 11.5 ! 7 | 9.25 | 11.5 ! 7 | 9.25 | 11.5 ! 7 | Infinity | Infinity ! 7.15 | Infinity | Infinity !7.2 | 10.35 | 13.5 !7.3 | 48.65 |90 !7.3 | Infinity | Infinity !7.3 | Infinity | Infinity !7.4 | 9.75 | 12.1 !7.4 | Infinity | Infinity !7.5 |9.5 | 11.5 !7.5 | 9.75 |12 !7.5 | Infinity | Infinity !7.7 |9.6 | 11.5 !7.7 | Infinity | Infinity ! 7.75 | Infinity | Infinity ! 8 | 9.85 | 11.7 ! 8 | 10 |12 ! 8 | 10.5 |13 !8.2 | Infinity | Infinity !8.3 | Infinity | Infinity !8.5 | 10 | 11.5 !8.5 | 10.5
Re: [PATCHES] float4/float8/int64 passed by value with tsearch fixup
Alvaro Herrera [EMAIL PROTECTED] writes: I assume this is just some dumb portability mistake on my part ... or perhaps the fact that the functions are still using v0 fmgr convention? Since they're v0, they'd have to explicitly know about the pass-by-ref status of float4. Did this patch include a compile-time choice of whether things could remain pass-by-ref? I rather imagine that some people out there will prefer to stay that way instead of fix their old v0 code. In the meantime, converting contrib/seg to v1 might be the best solution. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] float4/float8/int64 passed by value with tsearch fixup
Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] writes: I assume this is just some dumb portability mistake on my part ... or perhaps the fact that the functions are still using v0 fmgr convention? Since they're v0, they'd have to explicitly know about the pass-by-ref status of float4. Well, the previous code was doing some pallocs, and the new code is not: http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/contrib/seg/seg.c.diff?r1=1.20;r2=1.21 Did this patch include a compile-time choice of whether things could remain pass-by-ref? I rather imagine that some people out there will prefer to stay that way instead of fix their old v0 code. Hmm, nope. Do we really need that? I understand the backwards-compatibility argument, yet I wonder if it's worth the extra effort and code complexity. In the meantime, converting contrib/seg to v1 might be the best solution. Will do. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] float4/float8/int64 passed by value with tsearch fixup
Alvaro Herrera [EMAIL PROTECTED] writes: Tom Lane wrote: Since they're v0, they'd have to explicitly know about the pass-by-ref status of float4. Well, the previous code was doing some pallocs, and the new code is not: http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/contrib/seg/seg.c.diff?r1=1.20;r2=1.21 [ shrug... ] So, you missed something. Did this patch include a compile-time choice of whether things could remain pass-by-ref? I rather imagine that some people out there will prefer to stay that way instead of fix their old v0 code. Hmm, nope. Do we really need that? Given that we *have to* handle a compile-time choice for whether float8 is pass-by-ref, I should think that allowing a similar choice for float4 is perfectly sensible and not really more work (it'll just be a second instance of the same code pattern). I'm not at all sure it made sense to apply this portion of the patch separately. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] float4/float8/int64 passed by value with tsearch fixup
Tom Lane [EMAIL PROTECTED] writes: Alvaro Herrera [EMAIL PROTECTED] writes: Tom Lane wrote: Since they're v0, they'd have to explicitly know about the pass-by-ref status of float4. Well, the previous code was doing some pallocs, and the new code is not: http://anoncvs.postgresql.org/cvsweb.cgi/pgsql/contrib/seg/seg.c.diff?r1=1.20;r2=1.21 [ shrug... ] So, you missed something. Specifically, I think what you missed is that on some platforms C functions pass or return float values differently from similar-sized integer or pointer values (typically, the float values get passed in floating-point registers). On such platforms it is essentially impossible for a v0 function to work with pass-by-val float4 --- since fmgr_oldstyle thinks it's passing integers and getting back pointers, the values are being transferred in the wrong registers. The only way to make it work would be to mangle the function declarations and then play union tricks like those in Float4GetDatum/DatumGetFloat4, which is certainly not very practical. It'd be less ugly to convert to v1 calling convention. (This very likely explains why the Berkeley folk made float4 pass-by-ref in the first place, a decision that doesn't make a lot of sense if you don't know about this problem.) So I think we really do need to offer that compile-time option. Failing to do so will be tantamount to desupporting v0 functions altogether, which I don't think we're prepared to do. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] float4/float8/int64 passed by value with tsearch fixup
Tom Lane wrote: Specifically, I think what you missed is that on some platforms C functions pass or return float values differently from similar-sized integer or pointer values (typically, the float values get passed in floating-point registers). Argh ... I would have certainly missed that. It'd be less ugly to convert to v1 calling convention. Okay -- I'll change contrib/seg to v1 to greenify back the buildfarm. So I think we really do need to offer that compile-time option. Failing to do so will be tantamount to desupporting v0 functions altogether, which I don't think we're prepared to do. I have asked the Cybertec guys for a patch. Since it's basically a copy of the float8 change, it should be easy to do. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Testing pg_terminate_backend()
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: Attached is my test script. I ran it for 14 hours (asserts on), running 450 regression tests, with up to seven backends killed per regression test. Hmm, there are something on the order of 1 SQL commands in our regression tests, so even assuming perfect randomness you've exercised SIGTERM on maybe 10% of them --- and of course there's multiple places in a complex DDL command where SIGTERM might conceivably be a problem. Who was volunteering to run this 24x7 for awhile? That was me. As long as the script runs properly on linux, I can get that started as soon as I'm fed instructions on how to do it :-) Do I just fix the paths and set it running, or do I need to prepare something else? SLEEP=`expr $RANDOM \* $REGRESSION_DURATION / 32767` Uh, where's the randomness coming from? ... but I should probably wait until that one is answered or fixed, I guess :-) //Magnus -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Testing pg_terminate_backend()
Magnus Hagander wrote: Tom Lane wrote: SLEEP=`expr $RANDOM \* $REGRESSION_DURATION / 32767` Uh, where's the randomness coming from? ... but I should probably wait until that one is answered or fixed, I guess :-) bash. RANDOM Each time this parameter is referenced, a random integer between 0 and 32767 is generated. The sequence of random numbers may be initialized by assigning a value to RANDOM. If RANDOM is unset, it loses its special properties, even if it is subsequently reset. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Testing pg_terminate_backend()
Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: Attached is my test script. I ran it for 14 hours (asserts on), running 450 regression tests, with up to seven backends killed per regression test. Hmm, there are something on the order of 1 SQL commands in our regression tests, so even assuming perfect randomness you've exercised SIGTERM on maybe 10% of them --- and of course there's multiple places in a complex DDL command where SIGTERM might conceivably be a problem. Who was volunteering to run this 24x7 for awhile? Yes, that is what it needs. SLEEP=`expr $RANDOM \* $REGRESSION_DURATION / 32767` Uh, where's the randomness coming from? In bash $RANDOM returns a random number from 0-32k every time; #!/bin/bash is specified in the top line. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Testing pg_terminate_backend()
Magnus Hagander wrote: Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: Attached is my test script. I ran it for 14 hours (asserts on), running 450 regression tests, with up to seven backends killed per regression test. Hmm, there are something on the order of 1 SQL commands in our regression tests, so even assuming perfect randomness you've exercised SIGTERM on maybe 10% of them --- and of course there's multiple places in a complex DDL command where SIGTERM might conceivably be a problem. Who was volunteering to run this 24x7 for awhile? That was me. As long as the script runs properly on linux, I can get that started as soon as I'm fed instructions on how to do it :-) Do I just fix the paths and set it running, or do I need to prepare something else? Nothing special to prepare. Compile with asserts enabled, and run the script. The comment at the top explains how to analyze the log for interesting error messages. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] float4/float8/int64 passed by value with tsearchfixup
Alvaro Herrera [EMAIL PROTECTED] writes: Tom Lane wrote: Specifically, I think what you missed is that on some platforms C functions pass or return float values differently from similar-sized integer or pointer values (typically, the float values get passed in floating-point registers). Argh ... I would have certainly missed that. Hum. That's a valid concern for some platforms, Sparc I think? But I'm skeptical that it would hit such a wide swathe of the build farm. In particular AFAIK the standard ABI for i386 does no such thing. You can get behaviour like that from GCC using function attributes like regparam but it's not the default. -- Gregory Stark EnterpriseDB http://www.enterprisedb.com Ask me about EnterpriseDB's Slony Replication support! -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] float4/float8/int64 passed by value with tsearchfixup
Gregory Stark [EMAIL PROTECTED] writes: Tom Lane wrote: Specifically, I think what you missed is that on some platforms C functions pass or return float values differently from similar-sized integer or pointer values (typically, the float values get passed in floating-point registers). Hum. That's a valid concern for some platforms, Sparc I think? But I'm skeptical that it would hit such a wide swathe of the build farm. I was wondering about that too, once it became obvious that (almost?) everything was failing not just some platforms. However, this afternoon's CVS HEAD *does* pass the seg regression test for me on HPPA, and I presume it passed on whatever Alvaro is using (btw, what was that?). So there's definitely a platform dependency involved and not just you-missed-a-pointer-someplace. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] float4/float8/int64 passed by value with tsearchfixup
Tom Lane wrote: Gregory Stark [EMAIL PROTECTED] writes: Tom Lane wrote: Specifically, I think what you missed is that on some platforms C functions pass or return float values differently from similar-sized integer or pointer values (typically, the float values get passed in floating-point registers). Hum. That's a valid concern for some platforms, Sparc I think? But I'm skeptical that it would hit such a wide swathe of the build farm. I was wondering about that too, once it became obvious that (almost?) everything was failing not just some platforms. However, this afternoon's CVS HEAD *does* pass the seg regression test for me on HPPA, and I presume it passed on whatever Alvaro is using (btw, what was that?). So there's definitely a platform dependency involved and not just you-missed-a-pointer-someplace. Yeah, it passed for me -- this is a dual-core AMD X2 (amd64), gcc 4.1.3. Also, it didn't fail in the same way everywhere: for example, fennec (x86-64, gcc 4.1) shows everything returned as zeros instead of random values that most other platforms show. Further fumbling says that fennec, chinchilla, heron, dungbeetle and platypus display zeroes. These are all the AMD64 machines (different operating systems: mostly Linux, one FreeBSD) that managed to run during the time that the bug was out there. Dugong failed differently, returning NaN. The others returned random pointers, all in the same area (they all display the same power of 10 values, presumably random but nearby stack or data addresses or something like that). http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=fennecdt=2008-04-18%2021:10:01 *** *** 1070,1218 FROM test_seg WHERE s @ '11.2..11.3' OR s IS NULL ORDER BY s; seg_lower | seg_center | seg_upper ---++--- ! -Infinity | -Infinity |40 ! -Infinity | -Infinity |82 ! -Infinity | -Infinity |90 ! 1 | 7 |13 !1.3 | 6.65 |12 ! 2 | 6.75 | 11.5 !2.1 | 6.95 | 11.8 !2.3 | Infinity | Infinity !2.3 | Infinity | Infinity !2.4 | 6.85 | 11.3 !2.5 | 7 | 11.5 !2.5 | 7.15 | 11.8 !2.6 | Infinity | Infinity !2.7 | 7.35 |12 ! 3 | Infinity | Infinity ! 3 | 30.5 |58 !3.1 |7.3 | 11.5 !3.5 |7.5 | 11.5 !3.5 | 7.85 | 12.2 ! 4 | 8 |12 ! 4 | Infinity | Infinity ! 4 | 8 |12 ! 4 | 7.85 | 11.7 ! 4 | 8.25 | 12.5 ! 4 |8.5 |13 ! 4 | 32 |60 ! 4 | Infinity | Infinity !4.2 | 7.85 | 11.5 !4.2 | 7.95 | 11.7 !4.5 | 8.25 |12 !4.5 | 8 | 11.5 !4.5 | 8.25 |12 !4.5 | 8.25 |12 !4.5 |8.5 | 12.5 !4.5 | 59.75 | 115 !4.7 | 8.25 | 11.8 !4.8 | 8.15 | 11.5 !4.8 |8.2 | 11.6 !4.8 | 8.65 | 12.5 !4.8 | Infinity | Infinity !4.9 | 8.45 |12 !4.9 | Infinity | Infinity ! 5 | 8.25 | 11.5 ! 5 |8.5 |12 ! 5 | 17.5 |30 ! 5 |8.2 | 11.4 ! 5 | 8.25 | 11.5 ! 5 |8.3 | 11.6 ! 5 | 8.35 | 11.7 ! 5 |8.5 |12 ! 5 |8.5 |12 ! 5 |8.5 |12 !5.2 | 8.35 | 11.5 !5.2 |8.6 |12 ! 5.25 | 8.625 |12 !5.3 |8.4 | 11.5 !5.3 | 9.15 |13 !5.3 | 47.65 |90 !5.3 | Infinity | Infinity !5.4 | Infinity | Infinity !5.5 |8.5 | 11.5 !5.5 |8.6 | 11.7 !5.5 | 8.75 |12 !5.5 | 8.75 |12 !5.5 | 9 | 12.5 !5.5 |9.5 | 13.5 !5.5 | Infinity | Infinity !5.5 | Infinity | Infinity !5.7 | Infinity | Infinity !5.9 | Infinity | Infinity ! 6 | 8.75 | 11.5 ! 6 | 9 |12 ! 6 | 8.75 | 11.5 ! 6 |9.5 |13 ! 6 | 8.75 | 11.5 !6.1 | 9.05 |12 !6.1 | Infinity | Infinity !6.2 | 8.85 | 11.5 !6.3 | Infinity | Infinity !6.5 | 9 | 11.5 !6.5 | 9.25 |12 !6.5 | 9.25 |12 !6.5 | Infinity | Infinity !
Re: [PATCHES] float4/float8/int64 passed by value with tsearchfixup
Tom Lane wrote: I was wondering about that too, once it became obvious that (almost?) everything was failing not just some platforms. However, this afternoon's CVS HEAD *does* pass the seg regression test for me on HPPA, and I presume it passed on whatever Alvaro is using (btw, what was that?). So there's definitely a platform dependency involved and not just you-missed-a-pointer-someplace. Huh, this was with the code with v0 conventions, right? I committed the change to v1 conventions a while ago. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Testing pg_terminate_backend()
bruce wrote: Magnus Hagander wrote: Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: Attached is my test script. I ran it for 14 hours (asserts on), running 450 regression tests, with up to seven backends killed per regression test. Hmm, there are something on the order of 1 SQL commands in our regression tests, so even assuming perfect randomness you've exercised SIGTERM on maybe 10% of them --- and of course there's multiple places in a complex DDL command where SIGTERM might conceivably be a problem. Who was volunteering to run this 24x7 for awhile? That was me. As long as the script runs properly on linux, I can get that started as soon as I'm fed instructions on how to do it :-) Do I just fix the paths and set it running, or do I need to prepare something else? Nothing special to prepare. Compile with asserts enabled, and run the script. The comment at the top explains how to analyze the log for interesting error messages. Oh, you need to set a variable in the script indicating the average number of seconds it takes to run the regression test on your system. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] Testing pg_terminate_backend()
bruce wrote: bruce wrote: Magnus Hagander wrote: Tom Lane wrote: Bruce Momjian [EMAIL PROTECTED] writes: Attached is my test script. I ran it for 14 hours (asserts on), running 450 regression tests, with up to seven backends killed per regression test. Hmm, there are something on the order of 1 SQL commands in our regression tests, so even assuming perfect randomness you've exercised SIGTERM on maybe 10% of them --- and of course there's multiple places in a complex DDL command where SIGTERM might conceivably be a problem. Who was volunteering to run this 24x7 for awhile? That was me. As long as the script runs properly on linux, I can get that started as soon as I'm fed instructions on how to do it :-) Do I just fix the paths and set it running, or do I need to prepare something else? Nothing special to prepare. Compile with asserts enabled, and run the script. The comment at the top explains how to analyze the log for interesting error messages. Oh, you need to set a variable in the script indicating the average number of seconds it takes to run the regression test on your system. And you have to set the location of the output file and where your regression test directory is located. -- Bruce Momjian [EMAIL PROTECTED]http://momjian.us EnterpriseDB http://enterprisedb.com + If your life is a hard drive, Christ can be your backup. + -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
Re: [PATCHES] float4/float8/int64 passed by value with tsearchfixup
Alvaro Herrera [EMAIL PROTECTED] writes: Tom Lane wrote: I was wondering about that too, once it became obvious that (almost?) everything was failing not just some platforms. However, this afternoon's CVS HEAD *does* pass the seg regression test for me on HPPA, and I presume it passed on whatever Alvaro is using (btw, what was that?). So there's definitely a platform dependency involved and not just you-missed-a-pointer-someplace. Huh, this was with the code with v0 conventions, right? I committed the change to v1 conventions a while ago. Right, I had intentionally cvs updated to the broken version to test. regards, tom lane -- Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-patches
