Re: [HACKERS] Improve OOM handling in pg_locale.c
Mithun Cywrites: > On Thu, Oct 13, 2016 at 1:40 PM, Michael Paquier > wrote: >> I am attaching that to the next CF. > One thing which you might need to reconsider is removal of memory leak > comments. There is still a leak if there is an error while encoding in > db_encoding_strdup. > Unless you want to catch those error with an TRY();CATCH(); and then > free the mem. I could have lived with leaving the leak there, but really this wasn't fixing the worst problem with the code: if it did throw an error out of the middle of that sequence, it would leave the process setlocale'd to some other locale than we want. That could lead to unwanted behavior in printf and other functions. And this isn't all that unlikely: an encoding conversion failure is definitely possible if you have a locale selected that's not compatible with the database encoding. I whacked the patch around enough so that we didn't do anything except libc calls between setting and restoring the locale. At that point it was just a matter of adding a TRY block to be able to say that we didn't leak any strdup'd strings, so I figured "might as well". Pushed with those changes. regards, tom lane -- 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] Improve OOM handling in pg_locale.c
On Tue, Nov 22, 2016 at 8:28 AM, Tom Lanewrote: > I could have lived with leaving the leak there, but really this wasn't > fixing the worst problem with the code: if it did throw an error out of > the middle of that sequence, it would leave the process setlocale'd to > some other locale than we want. That could lead to unwanted behavior > in printf and other functions. And this isn't all that unlikely: an > encoding conversion failure is definitely possible if you have a locale > selected that's not compatible with the database encoding. > > I whacked the patch around enough so that we didn't do anything except > libc calls between setting and restoring the locale. At that point it > was just a matter of adding a TRY block to be able to say that we > didn't leak any strdup'd strings, so I figured "might as well". > > Pushed with those changes. Thanks. The changes you have done look good to me at short sight. -- Michael -- 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] Improve OOM handling in pg_locale.c
On Thu, Oct 13, 2016 at 1:40 PM, Michael Paquierwrote: > I am attaching that to the next CF. I have tested this patch. Now we error out as OOM instead of crash. postgres=# SELECT '12.34'::money; ERROR: out of memory LINE 1: SELECT '12.34'::money; One thing which you might need to reconsider is removal of memory leak comments. There is still a leak if there is an error while encoding in db_encoding_strdup. Unless you want to catch those error with an TRY();CATCH(); and then free the mem. -* localeconv()'s results. Note that if we were to fail within this -* sequence before reaching "CurrentLocaleConvAllocated = true", we could -* leak some memory --- but not much, so it's not worth agonizing over. Rest all LGTM. -- Thanks and Regards Mithun C Y EnterpriseDB: http://www.enterprisedb.com
Re: [HACKERS] Improve OOM handling in pg_locale.c
Hi Mithun, This is a gentle reminder. you assigned as reviewer to the current patch in the 11-2016 commitfest. But you haven't shared your review yet. Can you please try to share your views about the patch. This will help us in smoother operation of commitfest. Please Ignore if you already shared your review. Regards, Hari Babu Fujitsu Australia