ECPG - Remove need for "AT connection" when using threads

Shridhar, Once the patches I've put forward are applied there's still
a further change I've got planned which will remove the mutex locking
in the common case - a NULL/DEFAULT connection parameter (I'll post a
patch soon). This leaves the threaded case with comparable performance
to the non-threaded case (both have a function call to get the
connection, plus a getspecific call in the threaded case). As such is
there much benefit in adding support for the connection being supplied
by a struct pointer? You'd also have to add in something like "EXEC
SQL GET DESCRIPTION xxx" to get the pointer too. How would it improve
things over how they are in the test_thread_implicit test program?

I still think it's worthwhile investigating the use of GCC's __thread
storage class specifier to remove the use of pthread_*specific in this
case. This would also be a help to the WIN32 port since this specifier
maps well to similar constructs in Microsoft's and Borland's compilers
(see "thread" item in the TODO at developer.postgresql.org).

And I still can't see how you'll bind sqlca to the connection object,
but best of luck!

Regards, Lee K.

Lee Kindness wrote:

Shridhar, Once the patches I've put forward are applied there's still
a further change I've got planned which will remove the mutex locking
in the common case - a NULL/DEFAULT connection parameter (I'll post a
patch soon). This leaves the threaded case with comparable performance
to the non-threaded case (both have a function call to get the
connection, plus a getspecific call in the threaded case). As such is
there much benefit in adding support for the connection being supplied
by a struct pointer? You'd also have to add in something like "EXEC
SQL GET DESCRIPTION xxx" to get the pointer too. How would it improve
things over how they are in the test_thread_implicit test program?

I still think it's worthwhile investigating the use of GCC's __thread
storage class specifier to remove the use of pthread_*specific in this
case. This would also be a help to the WIN32 port since this specifier
maps well to similar constructs in Microsoft's and Borland's compilers
(see "thread" item in the TODO at developer.postgresql.org).

I would like to avoid compiler-specific thread stuff unless tests can
show a performance benefit.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

From: "Bruce Momjian" <pgman@candle.pha.pa.us>

Lee Kindness wrote:

I still think it's worthwhile investigating the use of GCC's __thread
storage class specifier to remove the use of pthread_*specific in this
case. This would also be a help to the WIN32 port since this specifier
maps well to similar constructs in Microsoft's and Borland's compilers
(see "thread" item in the TODO at developer.postgresql.org).

I would like to avoid compiler-specific thread stuff unless tests can
show a performance benefit.

I think concerns re performance are largely moot - with the thread specific
data performance is much the same as without. However native compiler
support for thread specific data is much more straightforward and
understandable - you simply end up with a variable that can be used like any
other except there is a copy of it for each thread.

To make ECPG thread-safe for WIN32 an additional set of thread calls will
need to be added, and/or similar features to GCC's __thread storage
specifier. If we end up adding these for WIN32 then it may as well be done
for GCC too. I probably will experiment with it a bit (and get some real
performance figure, rather than my hunch!)...

Perhaps a cleaner way is to use an existing thread package with encompasses
the various platform APIs - i.e. APR or ACE, or... But that's a big
discussion, and not one I'm keen to get into at the moment. A more
appropriate time is perhaps once the WIN32 port is completed? It would also
be straightforward to encompass this in an PostgreSQL specific API to wrap
around the various calls we use and, if available, make these no-ops when a
suitable storage class is supplied by the compiler? I'd be happy to write
this API if others saw it as a way forward.

Perhaps someone would like to fwd this on to the hackers-win32 list (I'm not
subscribed) to see what their view is on thread safety in the client
libraries? And what approach they're planning?

L.

Lee Kindness wrote:

From: "Bruce Momjian" <pgman@candle.pha.pa.us>

Lee Kindness wrote:

I still think it's worthwhile investigating the use of GCC's __thread
storage class specifier to remove the use of pthread_*specific in this
case. This would also be a help to the WIN32 port since this specifier
maps well to similar constructs in Microsoft's and Borland's compilers
(see "thread" item in the TODO at developer.postgresql.org).

I would like to avoid compiler-specific thread stuff unless tests can
show a performance benefit.

I think concerns re performance are largely moot - with the thread specific
data performance is much the same as without. However native compiler
support for thread specific data is much more straightforward and
understandable - you simply end up with a variable that can be used like any
other except there is a copy of it for each thread.

To make ECPG thread-safe for WIN32 an additional set of thread calls will
need to be added, and/or similar features to GCC's __thread storage
specifier. If we end up adding these for WIN32 then it may as well be done
for GCC too. I probably will experiment with it a bit (and get some real
performance figure, rather than my hunch!)...

Perhaps a cleaner way is to use an existing thread package with encompasses
the various platform APIs - i.e. APR or ACE, or... But that's a big
discussion, and not one I'm keen to get into at the moment. A more
appropriate time is perhaps once the WIN32 port is completed? It would also
be straightforward to encompass this in an PostgreSQL specific API to wrap
around the various calls we use and, if available, make these no-ops when a
suitable storage class is supplied by the compiler? I'd be happy to write
this API if others saw it as a way forward.

Perhaps someone would like to fwd this on to the hackers-win32 list (I'm not
subscribed) to see what their view is on thread safety in the client
libraries? And what approach they're planning?

I guess my point was that if there isn't a big performance win, why do
compiler specific and POSIX standard both in the same code. The
compiler-specific might be clearer, but if we have to support non-gcc
too, which we do, adding a cleaner solution to one that is already
standard actually makes it look worse.

I don't think MinGW support thread-specific right now (no TLS support),
so we will need native Win32 in there anyway. Adding a third seems like
more confusion.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

From: "Bruce Momjian" <pgman@candle.pha.pa.us>

Lee Kindness wrote:

Perhaps a cleaner way is to use an existing thread package with

encompasses

the various platform APIs - i.e. APR or ACE, or... But that's a big
discussion, and not one I'm keen to get into at the moment. A more
appropriate time is perhaps once the WIN32 port is completed? It would

also

be straightforward to encompass this in an PostgreSQL specific API to

wrap

around the various calls we use and, if available, make these no-ops

when a

suitable storage class is supplied by the compiler? I'd be happy to

write

this API if others saw it as a way forward.

I don't think MinGW support thread-specific right now (no TLS support),
so we will need native Win32 in there anyway. Adding a third seems like
more confusion.

Ah, ok - i've not been following the win32 stuff so wasn't even sure on
compilers being used. I'd agree at this stage there's no point muddying the
waters even further!

I'll get back to you with the patch to move common-case connection retrieval
outwith the mutex once the earlier patches are applied.

Thanks, L.

On Sun, Mar 07, 2004 at 09:43:31PM -0000, Lee Kindness wrote:

Attached diff improves the handling of connections in ecpg when threading is
used. The "current connection" is now the last connection made/set for the

I just applied this patch and the last one.

Michael
--
Michael Meskes
Email: Michael at Fam-Meskes dot De
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org
Go SF 49ers! Go Rhein Fire! Use Debian GNU/Linux! Use PostgreSQL!

Patch applied by Michael. Thanks.

---------------------------------------------------------------------------

Lee Kindness wrote:

Attached diff improves the handling of connections in ecpg when threading is
used. The "current connection" is now the last connection made/set for the
thread in question - not the absolute last connection made/set (over all
threads in the process). This change removes the tedious need to always use
"AT connection" for each and every EXEC SQL call (an item on the TODO).

The "test_thread_implicit" test program added in the last email will
function as intended with this patch.

Thanks, L.

----- Original Message -----
From: Lee Kindness
To: pgsql-patches@postgresql.org
Cc: Bruce Momjian ; Lee Kindness ; Shridhar Daithankar
Sent: Saturday, March 06, 2004 3:07 PM
Subject: ECPG: Update tests & memory leak fix

See attached diff which corrects a per-thread memory leak in libecpg and
changes the thread test program to create a user defined number of threads.
Also attached tar.gz adds an additional thread test, which currently does
NOT work as expected and is one of the cases Shridar and I will be looking
at (removing need for AT xxx on every EXEC SQL).

Note, the updated test shows that ECPG currently DOES support the
specification of connection names by host variables (not only static strings
as I had said previously).

Thanks, L.

[ Attachment, skipping... ]

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Michael Meskes wrote:

On Sun, Mar 07, 2004 at 09:43:31PM -0000, Lee Kindness wrote:

Attached diff improves the handling of connections in ecpg when threading is
used. The "current connection" is now the last connection made/set for the

I just applied this patch and the last one.

I assume you mean you applied:

Update tests & memory leak fix

and

ECPG - Remove need for "AT connection" when using threads

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

On Sun, Mar 14, 2004 at 09:11:27AM -0500, Bruce Momjian wrote:

I just applied this patch and the last one.

I assume you mean you applied:

Update tests & memory leak fix

and

ECPG - Remove need for "AT connection" when using threads

Yes. Sorry, should have said so.

Michael
--
Michael Meskes
Email: Michael at Fam-Meskes dot De
ICQ: 179140304, AIM/Yahoo: michaelmeskes, Jabber: meskes@jabber.org
Go SF 49ers! Go Rhein Fire! Use Debian GNU/Linux! Use PostgreSQL!

The "cvs add" of test_thread_implicit.pgc seems to have been missed,
i've attached this again.

Additionally I include a small patch to remove mutex locking when a
DEFAULT/NULL connection is being retrieved. This is consistent with
libpq.

Thanks, L.

Michael Meskes writes:

Patch applied. File added. Thanks.

---------------------------------------------------------------------------

Lee Kindness wrote:
Content-Description: message body text

The "cvs add" of test_thread_implicit.pgc seems to have been missed,
i've attached this again.

Additionally I include a small patch to remove mutex locking when a
DEFAULT/NULL connection is being retrieved. This is consistent with
libpq.

Thanks, L.

Michael Meskes writes:

On Sun, Mar 14, 2004 at 09:11:27AM -0500, Bruce Momjian wrote:

I just applied this patch and the last one.

I assume you mean you applied:

Update tests & memory leak fix

and

ECPG - Remove need for "AT connection" when using threads

Yes. Sorry, should have said so.

Michael

/*
* Thread test program
* by Lee Kindness.
*/

/* #define ECPGDEBUG */

#include <pthread.h>
#include <stdlib.h>

void *test_thread(void *arg);

EXEC SQL BEGIN DECLARE SECTION;
char *l_dbname;
EXEC SQL END DECLARE SECTION;
int nthreads = 2;
int iterations = 10;

int main(int argc, char *argv[])
{
#ifdef ECPGDEBUG
char debugfilename[] = "thread_test_implicit.log";
FILE *debugfile;
#endif
pthread_t *threads;
int n;
EXEC SQL BEGIN DECLARE SECTION;
int l_rows;
EXEC SQL END DECLARE SECTION;

/* parse command line arguments */
if( (argc < 2) || (argc > 4) )
{
fprintf(stderr, "Usage: %s dbname [threads] [iterations_per_thread]\n", argv[0]);
return( 1 );
}
l_dbname = argv[1];
if( argc >= 3 )
nthreads = atoi(argv[2]);
if( argc == 4 )
iterations = atoi(argv[3]);

/* open ECPG debug log? */
#ifdef ECPGDEBUG
debugfile = fopen(debugfilename, "w");
if( debugfile != NULL )
ECPGdebug(1, debugfile);
else
fprintf(stderr, "Cannot open ECPG debug log: %s\n", debugfilename);
#endif

/* setup test_thread table */
EXEC SQL CONNECT TO:l_dbname;
EXEC SQL DROP TABLE test_thread; /* DROP might fail */
EXEC SQL COMMIT;
EXEC SQL CREATE TABLE
test_thread(tstamp TIMESTAMP NOT NULL DEFAULT CAST(timeofday() AS TIMESTAMP),
thread TEXT NOT NULL,
iteration INTEGER NOT NULL,
PRIMARY KEY(thread, iteration));
EXEC SQL COMMIT;
EXEC SQL DISCONNECT;

/* create, and start, threads */
threads = calloc(nthreads, sizeof(pthread_t));
if( threads == NULL )
{
fprintf(stderr, "Cannot alloc memory\n");
return( 1 );
}
for( n = 0; n < nthreads; n++ )
{
pthread_create(&threads[n], NULL, test_thread, (void *)n + 1);
}

/* wait for thread completion */
for( n = 0; n < nthreads; n++ )
{
pthread_join(threads[n], NULL);
}
free(threads);

/* and check results */
EXEC SQL CONNECT TO :l_dbname;
EXEC SQL SELECT COUNT(*) INTO :l_rows FROM test_thread;
EXEC SQL COMMIT;
EXEC SQL DISCONNECT;
if( l_rows == (nthreads * iterations) )
printf("\nSuccess.\n");
else
printf("\nERROR: Failure - expecting %d rows, got %d.\n", nthreads * iterations, l_rows);

/* close ECPG debug log? */
#ifdef ECPGDEBUG
if( debugfile != NULL )
{
ECPGdebug(0, debugfile);
fclose(debugfile);
}
#endif

return( 0 );
}

void *test_thread(void *arg)
{
long threadnum = (long)arg;
EXEC SQL BEGIN DECLARE SECTION;
int l_i;
char l_connection[128];
EXEC SQL END DECLARE SECTION;

/* build up connection name, and connect to database */
snprintf(l_connection, sizeof(l_connection), "thread_%03ld", threadnum);
EXEC SQL WHENEVER sqlerror sqlprint;
EXEC SQL CONNECT TO :l_dbname AS :l_connection;
if( sqlca.sqlcode != 0 )
{
printf("%s: ERROR: cannot connect to database!\n", l_connection);
return( NULL );
}
EXEC SQL BEGIN;

/* insert into test_thread table */
for( l_i = 1; l_i <= iterations; l_i++ )
{
printf("%s: inserting %d\n", l_connection, l_i);
EXEC SQL INSERT INTO test_thread(thread, iteration) VALUES(:l_connection, :l_i);
if( sqlca.sqlcode == 0 )
printf("%s: insert done\n", l_connection);
else
printf("%s: ERROR: insert failed!\n", l_connection);
}

/* all done */
EXEC SQL COMMIT;
EXEC SQL DISCONNECT :l_connection;
printf("%s: done!\n", l_connection);
return( NULL );
}

Index: src/interfaces/ecpg/ecpglib/connect.c
===================================================================
RCS file: /projects/cvsroot/pgsql-server/src/interfaces/ecpg/ecpglib/connect.c,v
retrieving revision 1.20
diff -c -r1.20 connect.c
*** src/interfaces/ecpg/ecpglib/connect.c	14 Mar 2004 12:16:29 -0000	1.20
--- src/interfaces/ecpg/ecpglib/connect.c	15 Mar 2004 16:17:36 -0000
***************
*** 62,79 ****
{
struct connection *ret = NULL;

#ifdef ENABLE_THREAD_SAFETY
! pthread_mutex_lock(&connections_mutex);
#endif

! ret = ecpg_get_connection_nr(connection_name);

#ifdef ENABLE_THREAD_SAFETY
! pthread_mutex_unlock(&connections_mutex);
#endif

return (ret);
-
}

static void
--- 62,89 ----
{
struct connection *ret = NULL;

+ 	if ((connection_name == NULL) || (strcmp(connection_name, "CURRENT") == 0))
+ 	{
#ifdef ENABLE_THREAD_SAFETY
! 		ret = pthread_getspecific(actual_connection_key);
! #else
! 		ret = actual_connection;
! #endif
! 	}
! 	else
! 	{
! #ifdef ENABLE_THREAD_SAFETY
! 		pthread_mutex_lock(&connections_mutex);
#endif

! ret = ecpg_get_connection_nr(connection_name);

#ifdef ENABLE_THREAD_SAFETY
! pthread_mutex_unlock(&connections_mutex);
#endif
+ }

return (ret);
}

static void

---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073