Re: [PATCHES] float4/float8/int64 passed by value with tsearch fixup
On Mon, Apr 21, 2008 at 7:10 AM, Tom Lane [EMAIL PROTECTED] wrote: Note that we are already exercising both ends of the float8-byval option, so probably only --disable-float4-byval is very interesting to test explicitly. I'll set up a new animal with --disable-float4-byval this week. -- Guillaume -- 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 [EMAIL PROTECTED] writes: I tried to split the previous patch up to see where the tsearch regression comes from. So, it turned out that: - float4 conversion is risk free (patch #1) - float8 conversion is okay, too, if coupled with time[stamp[tz]] conversion (patch #2) but with int64 timestamps enabled, the next one is also needed: - int64 conversion (patch #3) is mostly okay but it is the one that's causing the tsearch regression Applied with revisions --- mostly, supporting configure-time control over whether pass-by-value is used. 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
On Mon, Apr 21, 2008 at 2:28 AM, Tom Lane [EMAIL PROTECTED] wrote: Applied with revisions --- mostly, supporting configure-time control over whether pass-by-value is used. Do we need buildfarm coverage of these options? -- Guillaume -- 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
Guillaume Smet [EMAIL PROTECTED] writes: On Mon, Apr 21, 2008 at 2:28 AM, Tom Lane [EMAIL PROTECTED] wrote: Applied with revisions --- mostly, supporting configure-time control over whether pass-by-value is used. Do we need buildfarm coverage of these options? Wouldn't hurt, I guess, though it's not very urgent since the non-default cases were default up till these patches. Note that we are already exercising both ends of the float8-byval option, so probably only --disable-float4-byval is very interesting to test explicitly. 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
Alvaro Herrera írta: 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. Here's the patch (against current CVS) with the required change. Please review, it passed make check with both --enable and --disable-float4-byval. -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH http://www.postgresql.at/ 01-pg84-passedbyval-float4-config.patch.gz Description: Unix tar archive -- 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: 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. Here's the patch (against current CVS) with the required change. Please review, it passed make check with both --enable and --disable-float4-byval. Does it pass make installcheck in contrib? I'm worried about btree_gist in particular. Perhaps the change I introduced in the previous revision needs to be #ifdef'd out? -- 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] float4/float8/int64 passed by value with tsearch fixup
Alvaro Herrera írta: Zoltan Boszormenyi wrote: 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. Here's the patch (against current CVS) with the required change. Please review, it passed make check with both --enable and --disable-float4-byval. Does it pass make installcheck in contrib? I'm worried about It seems to pass, see below. btree_gist in particular. Perhaps the change I introduced in the previous revision needs to be #ifdef'd out? Both --enable and --disable-float4-byval produced only this regression, but it seems to be a tuple order difference. = running regression test queries== test tsearch2 ... FAILED cat tsearch2/regression.diffs: *** ./expected/tsearch2.out Tue Nov 20 05:23:10 2007 --- ./results/tsearch2.out Sat Apr 19 20:48:16 2008 *** *** 1490,1497 w0| 29 | 29 y9| 29 | 29 zm| 29 | 29 - zs| 29 | 29 zy| 29 | 29 ax| 28 | 28 cd| 28 | 28 dj| 28 | 28 --- 1490,1497 w0| 29 | 29 y9| 29 | 29 zm| 29 | 29 zy| 29 | 29 + zs| 29 | 29 ax| 28 | 28 cd| 28 | 28 dj| 28 | 28 == -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH http://www.postgresql.at/ -- 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 [EMAIL PROTECTED] writes: Both --enable and --disable-float4-byval produced only this regression, but it seems to be a tuple order difference. That looks suspiciously locale-ish; what locale are you running PG in? 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 írta: Zoltan Boszormenyi [EMAIL PROTECTED] writes: Both --enable and --disable-float4-byval produced only this regression, but it seems to be a tuple order difference. That looks suspiciously locale-ish; what locale are you running PG in? regards, tom lane hu_HU.UTF-8 -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH http://www.postgresql.at/ -- 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: 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. Well, there might still be some references to them in add-on code, but considering that c.h has said this since 7.1 * XXX: these typedefs are now deprecated in favor of float4 and float8. * They will eventually go away. it seems hard to argue that we've not given adequate notice. I'm +1 for removing them, and float64/float64data too. It's not like they're hard to avoid using, even if you don't want to update to v1 call convention. If there are no objections, I'll see about pushing in the remaining parts of the patch (the original and the just-submitted fix to allow float4byval to be selectable) over this weekend. 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 [EMAIL PROTECTED] writes: That looks suspiciously locale-ish; what locale are you running PG in? hu_HU.UTF-8 Ah, and I'll bet zs sorts after zy in hu_HU. The query is already doing an ORDER BY, so it's not at fault. I think the only thing we could do about this is add a variant expected file with the hu_HU sort ordering. I'd be happy to do that if it were affecting the main regression tests, but not sure it's worth it for contrib/tsearch2 ... thoughts? 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 írta: Zoltan Boszormenyi [EMAIL PROTECTED] writes: That looks suspiciously locale-ish; what locale are you running PG in? hu_HU.UTF-8 Ah, and I'll bet zs sorts after zy in hu_HU. Yes, zs is a double letter that sorts after z in general un hu_HU. The query is already doing an ORDER BY, so it's not at fault. I think the only thing we could do about this is add a variant expected file with the hu_HU sort ordering. I'd be happy to do that if it were affecting the main regression tests, but not sure it's worth it for contrib/tsearch2 ... thoughts? regards, tom lane -- -- Zoltán Böszörményi Cybertec Schönig Schönig GmbH http://www.postgresql.at/ -- 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
