pg.ddx.io pgsql-hackers@postgresql.org mailing list archive help / color / mirror / Atom feed From: Noah Misch <noah@leadboat.com>
To: Guillaume Lelarge <guillaume@lelarge.info>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Benoit Lobréau <benoit.lobreau@gmail.com>
Cc: PostgreSQL Developers <pgsql-hackers@postgresql.org>
Cc: Dr. Michael Meskes <michael.meskes@credativ.com>
Subject: Re: Probable memory leak with ECPG and AIX
Date: Sun, 17 Apr 2022 21:16:45 -0700
Message-ID: <20220418041645.GA1134228@rfd.leadboat.com> (raw )
In-Reply-To: <20220102000750.GB54621@rfd.leadboat.com >
References: <CAECtzeWMVW=b8=DJje_4F=OG9BeYLh-phLcCxZ-FBpii+O=SBQ@mail.gmail.com >
<20211212073411.GA2218865@rfd.leadboat.com >
<CAPE8EZ4ca2s5tDVHZzQ=iGh+rD8VX5r4Gk8zTTEeh9VaDOMnuQ@mail.gmail.com >
<CAPE8EZ42eWAemjj58DO0WArCMDG2VYWut38aAGo9V8QH8ETqrg@mail.gmail.com >
<20220101074055.GA54621@rfd.leadboat.com >
<2865639.1641054902@sss.pgh.pa.us >
<20220102000750.GB54621@rfd.leadboat.com >
On Sat, Jan 01, 2022 at 04:07:50PM -0800, Noah Misch wrote:
> On Sat, Jan 01, 2022 at 11:35:02AM -0500, Tom Lane wrote:
> > Noah Misch <noah@leadboat.com> writes:
> > > I get the same results. The leak arises because AIX freelocale() doesn't free
> > > all memory allocated in newlocale(). The following program uses trivial
> > > memory on GNU/Linux, but it leaks like you're seeing on AIX:
> >
> > Bleah.
> >
> > > If you have access to file an AIX bug, I recommend doing so. If we want
> > > PostgreSQL to work around this, one idea is to have ECPG do this newlocale()
> > > less often. For example, do it once per process or once per connection
> > > instead of once per ecpg_do_prologue().
> >
> > It's worse than that: see also ECPGget_desc(). Seems like a case
> > could be made for doing something about this just on the basis
> > of cycles expended, never mind freelocale() bugs.
>
> Agreed. Once per process seems best. I only hesitated before since it means
> nothing will free this storage, which could be annoying in the context of
> Valgrind and similar. However, ECPG already has bits of never-freed memory in
> the form of pthread_key_create() calls having no pthread_key_delete(), so I
> don't mind adding a bit more.
The comparison to pthread_key_create() wasn't completely fair. While POSIX
welcomes pthread_key_create() to fail with ENOMEM, the glibc implementation
appears not to allocate memory. Even so, I'm okay leaking one newlocale() per
process lifetime.
I had expected to use pthread_once() for the newlocale() call, but there would
be no useful way to report failure and try again later. Instead, I called
newlocale() while ECPGconnect() holds connections_mutex. See log message and
comments for details. I tested "./configure ac_cv_func_uselocale=no ..." and
tested the scenario of newlocale() failing every time.
Attachments:
newlocale-once-ecpg-v1.patch (text/plain, 6.7K) download
Author: Noah Misch <noah@leadboat.com>
Commit: Noah Misch <noah@leadboat.com>
ecpglib: call newlocale() one per process.
ecpglib has been calling it once per SQL query and once per EXEC SQL GET
DESCRIPTOR. Instead, if newlocale() has not succeeded before, call it
while establishing a connection. This mitigates three problems:
- If newlocale() failed in EXEC SQL GET DESCRIPTOR, the command silently
proceeded without the intended locale change.
- On AIX, each newlocale()+freelocale() cycle leaked memory.
- newlocale() CPU usage may have been nontrivial.
Fail the connection attempt if newlocale() fails. Rerrange
ecpg_do_prologue() to validate the connection before its uselocale().
The sort of program that may regress is one running in an environment
where newlocale() fails. If that program establishes connections
without running SQL statements, it will stop working in response to this
change. I'm betting against the importance of such an ECPG use case.
Most SQL execution (any using ECPGdo()) has long required newlocale()
success, so there's little a connection could do without newlocale().
Back-patch to v10 (all supported versions).
Reviewed by FIXME. Reported by Guillaume Lelarge.
Discussion: https://postgr.es/m/20220101074055.GA54621@rfd.leadboat.com
diff --git a/src/interfaces/ecpg/ecpglib/connect.c b/src/interfaces/ecpg/ecpglib/connect.c
index 8dfcabe..2e4b003 100644
--- a/src/interfaces/ecpg/ecpglib/connect.c
+++ b/src/interfaces/ecpg/ecpglib/connect.c
@@ -10,6 +10,10 @@
#include "ecpgtype.h"
#include "sqlca.h"
+#ifdef HAVE_USELOCALE
+locale_t ecpg_clocale;
+#endif
+
#ifdef ENABLE_THREAD_SAFETY
static pthread_mutex_t connections_mutex = PTHREAD_MUTEX_INITIALIZER;
static pthread_key_t actual_connection_key;
@@ -502,6 +506,37 @@ ECPGconnect(int lineno, int c, const char *name, const char *user, const char *p
#ifdef ENABLE_THREAD_SAFETY
pthread_mutex_lock(&connections_mutex);
#endif
+#ifdef HAVE_USELOCALE
+ if (!ecpg_clocale)
+ {
+ ecpg_clocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0);
+ if (!ecpg_clocale)
+ {
+#ifdef ENABLE_THREAD_SAFETY
+ pthread_mutex_unlock(&connections_mutex);
+#endif
+ ecpg_raise(lineno, ECPG_OUT_OF_MEMORY,
+ ECPG_SQLSTATE_ECPG_OUT_OF_MEMORY, NULL);
+ if (host)
+ ecpg_free(host);
+ if (port)
+ ecpg_free(port);
+ if (options)
+ ecpg_free(options);
+ if (realname)
+ ecpg_free(realname);
+ if (dbname)
+ ecpg_free(dbname);
+ if (conn_keywords)
+ ecpg_free(conn_keywords);
+ if (conn_values)
+ ecpg_free(conn_values);
+ free(this);
+ return false;
+ }
+ }
+#endif
+
if (connection_name != NULL)
this->name = ecpg_strdup(connection_name, lineno);
else
diff --git a/src/interfaces/ecpg/ecpglib/descriptor.c b/src/interfaces/ecpg/ecpglib/descriptor.c
index 369c2f0..f1898de 100644
--- a/src/interfaces/ecpg/ecpglib/descriptor.c
+++ b/src/interfaces/ecpg/ecpglib/descriptor.c
@@ -486,9 +486,16 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
/* since the database gives the standard decimal point */
/* (see comments in execute.c) */
#ifdef HAVE_USELOCALE
- stmt.clocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0);
- if (stmt.clocale != (locale_t) 0)
- stmt.oldlocale = uselocale(stmt.clocale);
+
+ /*
+ * To get here, the above PQnfields() test must have found nonzero
+ * fields. One needs a connection to create such a descriptor. (EXEC
+ * SQL SET DESCRIPTOR can populate the descriptor's "items", but it
+ * can't change the descriptor's PQnfields().) Any successful
+ * connection initializes ecpg_clocale.
+ */
+ Assert(ecpg_clocale);
+ stmt.oldlocale = uselocale(ecpg_clocale);
#else
#ifdef HAVE__CONFIGTHREADLOCALE
stmt.oldthreadlocale = _configthreadlocale(_ENABLE_PER_THREAD_LOCALE);
@@ -504,8 +511,6 @@ ECPGget_desc(int lineno, const char *desc_name, int index,...)
#ifdef HAVE_USELOCALE
if (stmt.oldlocale != (locale_t) 0)
uselocale(stmt.oldlocale);
- if (stmt.clocale)
- freelocale(stmt.clocale);
#else
if (stmt.oldlocale)
{
diff --git a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
index 1a98dea..c438cfb 100644
--- a/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
+++ b/src/interfaces/ecpg/ecpglib/ecpglib_extern.h
@@ -59,6 +59,10 @@ struct ECPGtype_information_cache
enum ARRAY_TYPE isarray;
};
+#ifdef HAVE_USELOCALE
+extern locale_t ecpg_clocale; /* LC_NUMERIC=C */
+#endif
+
/* structure to store one statement */
struct statement
{
@@ -73,7 +77,6 @@ struct statement
struct variable *inlist;
struct variable *outlist;
#ifdef HAVE_USELOCALE
- locale_t clocale;
locale_t oldlocale;
#else
char *oldlocale;
diff --git a/src/interfaces/ecpg/ecpglib/execute.c b/src/interfaces/ecpg/ecpglib/execute.c
index 6a7ef0b..2ebe665 100644
--- a/src/interfaces/ecpg/ecpglib/execute.c
+++ b/src/interfaces/ecpg/ecpglib/execute.c
@@ -101,10 +101,7 @@ free_statement(struct statement *stmt)
free_variable(stmt->outlist);
ecpg_free(stmt->command);
ecpg_free(stmt->name);
-#ifdef HAVE_USELOCALE
- if (stmt->clocale)
- freelocale(stmt->clocale);
-#else
+#ifndef HAVE_USELOCALE
ecpg_free(stmt->oldlocale);
#endif
ecpg_free(stmt);
@@ -1965,6 +1962,15 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
return false;
}
+#ifdef ENABLE_THREAD_SAFETY
+ ecpg_pthreads_init();
+#endif
+
+ con = ecpg_get_connection(connection_name);
+
+ if (!ecpg_init(con, connection_name, lineno))
+ return false;
+
stmt = (struct statement *) ecpg_alloc(sizeof(struct statement), lineno);
if (stmt == NULL)
@@ -1979,13 +1985,13 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
* treat that situation as if the function doesn't exist.
*/
#ifdef HAVE_USELOCALE
- stmt->clocale = newlocale(LC_NUMERIC_MASK, "C", (locale_t) 0);
- if (stmt->clocale == (locale_t) 0)
- {
- ecpg_do_epilogue(stmt);
- return false;
- }
- stmt->oldlocale = uselocale(stmt->clocale);
+
+ /*
+ * Since ecpg_init() succeeded, we have a connection. Any successful
+ * connection initializes ecpg_clocale.
+ */
+ Assert(ecpg_clocale);
+ stmt->oldlocale = uselocale(ecpg_clocale);
if (stmt->oldlocale == (locale_t) 0)
{
ecpg_do_epilogue(stmt);
@@ -2004,18 +2010,6 @@ ecpg_do_prologue(int lineno, const int compat, const int force_indicator,
setlocale(LC_NUMERIC, "C");
#endif
-#ifdef ENABLE_THREAD_SAFETY
- ecpg_pthreads_init();
-#endif
-
- con = ecpg_get_connection(connection_name);
-
- if (!ecpg_init(con, connection_name, lineno))
- {
- ecpg_do_epilogue(stmt);
- return false;
- }
-
/*
* If statement type is ECPGst_prepnormal we are supposed to prepare the
* statement before executing them
view thread (20+ messages) latest in thread
Message-ID: <20220418041645.GA1134228@rfd.leadboat.com>
Permalink: ../20220418041645.GA1134228@rfd.leadboat.com/
Also on: postgresql.org/message-id/20220418041645.GA1134228@rfd.leadboat.com
copy link · copy postgr.es · copy git am
reply Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com, guillaume@lelarge.info, tgl@sss.pgh.pa.us, benoit.lobreau@gmail.com, michael.meskes@credativ.com
Subject: Re: Probable memory leak with ECPG and AIX
In-Reply-To: <20220418041645.GA1134228@rfd.leadboat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox