Skip site navigation (1)
Skip section navigation (2)
Peripheral Links
Re: pgsql: Revert the behavior of inet/cidr functions to not unpack the arg
Revert the behavior of inet/cidr functions to not unpack the arguments. I forgot to change the functions to use the PG_GETARG_INET_PP() macro, when I changed DatumGetInetP() to unpack the datum, like Datum*P macros usually do. Also, I screwed up the definition of the PG_GETARG_INET_PP() macro, and didn't notice because it wasn't used. This fixes the memory leak when sorting inet values, as reported by Jochen Erwied and debugged by Andres Freund. Backpatch to 8.3, like the previous patch that broke it. Branch ------ REL9_1_STABLE Details ------- http://git.postgresql.org/pg/commitdiff/6cf639dfbddbc44d027730ad1504886312bc905a Modified Files -------------- src/backend/utils/adt/network.c | 98 +++++++++++++++++++------------------- src/include/utils/inet.h | 2 +- 2 files changed, 50 insertions(+), 50 deletions(-)
Hi, On Monday, December 12, 2011 09:29:23 AM Heikki Linnakangas wrote: > Revert the behavior of inet/cidr functions to not unpack the arguments. Whats the plan to handle this wrt a stable release? We had one more report of this on irc and I got two calls from clients already. And I don't have that many clients. It drives machines to OOM very quickly which isn't exactly good for the reputation of pg's stable releases... I CC'ed the guy reporting the problem on irc. Andres
Andres Freund <andres(at)anarazel(dot)de> writes: > On Monday, December 12, 2011 09:29:23 AM Heikki Linnakangas wrote: >> Revert the behavior of inet/cidr functions to not unpack the arguments. > Whats the plan to handle this wrt a stable release? I was wondering whether we shouldn't revert the original patch altogether in the back branches. As far as I'd heard, there were no reports of the previous coding causing real trouble, and this episode demonstrates that there is a possibility to make things worse rather than better. I think maybe we'd better treat this change as something to make in HEAD only. regards, tom lane
On Monday, December 12, 2011 07:48:01 PM Tom Lane wrote: > Andres Freund <andres(at)anarazel(dot)de> writes: > > On Monday, December 12, 2011 09:29:23 AM Heikki Linnakangas wrote: > >> Revert the behavior of inet/cidr functions to not unpack the arguments. > > > > Whats the plan to handle this wrt a stable release? > > I was wondering whether we shouldn't revert the original patch > altogether in the back branches. As far as I'd heard, there were > no reports of the previous coding causing real trouble, and this > episode demonstrates that there is a possibility to make things > worse rather than better. I think maybe we'd better treat this > change as something to make in HEAD only. +1
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 On 12/12/2011 07:55 PM, Andres Freund wrote: > On Monday, December 12, 2011 07:48:01 PM Tom Lane wrote: >> Andres Freund <andres(at)anarazel(dot)de> writes: >>> On Monday, December 12, 2011 09:29:23 AM Heikki Linnakangas wrote: >>>> Revert the behavior of inet/cidr functions to not unpack the arguments. >>> >>> Whats the plan to handle this wrt a stable release? >> >> I was wondering whether we shouldn't revert the original patch >> altogether in the back branches. As far as I'd heard, there were >> no reports of the previous coding causing real trouble, and this >> episode demonstrates that there is a possibility to make things >> worse rather than better. I think maybe we'd better treat this >> change as something to make in HEAD only. > > +1 +1 This is a bug that really qualifies for an early 9.1.3. A simple create index on an inet column almost crashed our production server and everything got very unresponsive for a while. The process running the create index grew to over 24GB in a matter of a few minuttes. We managed to kill the process at the last moment before going empty for cached memory and swap. regards, - -- Rafael Martinez Guerrero Center for Information Technology University of Oslo, Norway PGP Public Key: http://folk.uio.no/rafael/ -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.10 (GNU/Linux) Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/ iEYEARECAAYFAk7mW50ACgkQBhuKQurGihTm8QCgmgIa8GVXfmLDjX68Gu460vZz yuoAn2grjPPxcyBsVzetxpXJmnK9I0qE =07YX -----END PGP SIGNATURE-----
On 12.12.2011 20:55, Andres Freund wrote: > On Monday, December 12, 2011 07:48:01 PM Tom Lane wrote: >> Andres Freund<andres(at)anarazel(dot)de> writes: >>> On Monday, December 12, 2011 09:29:23 AM Heikki Linnakangas wrote: >>>> Revert the behavior of inet/cidr functions to not unpack the arguments. >>> >>> Whats the plan to handle this wrt a stable release? >> >> I was wondering whether we shouldn't revert the original patch >> altogether in the back branches. As far as I'd heard, there were >> no reports of the previous coding causing real trouble, and this >> episode demonstrates that there is a possibility to make things >> worse rather than better. I think maybe we'd better treat this >> change as something to make in HEAD only. > +1 Zoltan's original complaint was real, and he bumped into it in 9.1 and 9.0. There are few people using these macros in third party code, so I doubt we'll hear many reports either way. The memory leak should be fixed now, and I don't foresee any new issues cropping up, so I'm not inclined to revert it anymore. Within PostgreSQL itself, I couldn't find any more references to these macros. If we do revert, I think we should still leave the DatumGetInetPP() macro in place. It would be identical to DatumGetInetP(), neither would unpack, but DatumGetInetPP() would be the preferred macro to use for that purpose. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com
On 12.12.2011 21:53, Rafael Martinez wrote: > On 12/12/2011 07:55 PM, Andres Freund wrote: >> On Monday, December 12, 2011 07:48:01 PM Tom Lane wrote: >>> Andres Freund<andres(at)anarazel(dot)de> writes: >>>> On Monday, December 12, 2011 09:29:23 AM Heikki Linnakangas wrote: >>>>> Revert the behavior of inet/cidr functions to not unpack the arguments. >>>> >>>> Whats the plan to handle this wrt a stable release? >>> >>> I was wondering whether we shouldn't revert the original patch >>> altogether in the back branches. As far as I'd heard, there were >>> no reports of the previous coding causing real trouble, and this >>> episode demonstrates that there is a possibility to make things >>> worse rather than better. I think maybe we'd better treat this >>> change as something to make in HEAD only. >> >> +1 > > +1 > > This is a bug that really qualifies for an early 9.1.3. Fair enough, but that is a different issue from whether the original patch should be reverted altogether. The memory leak has been fixed now, in any case. -- Heikki Linnakangas EnterpriseDB http://www.enterprisedb.com