Re: [HACKERS] Allow substitute allocators for PGresult.

2011-12-22 Thread Kyotaro HORIGUCHI
Hello, thank you for taking the time for comment.

At Wed, 21 Dec 2011 11:09:59 -0500, Robert Haas robertmh...@gmail.com wrote
 I find the names of the functions added here to be quite
 confusing and would suggest renaming them.  I expected
 PQgetAsCstring to do something similar to PQgetvalue, but the
 code is completely different,

To be honest, I've also felt that kind of perplexity. If the
problem is simply of the naming, I can propose the another name
PQreadAttValue... This is not so good too...

 But...

 and even after reading the documentation I still don't
 understand what that function is supposed to be used for.  Why
 as cstring?  What would the other option be?

Is it a problem of the poor description? Or about the raison
d'être of the function?

The immediate cause of the existence of the function is that
getAnotherTuple internally stores the field values of the tuples
sent from the server, in the form of PGresAttValue, and I have
found only one route to store a tuple into TupleStore is
BuildeTupleFromCStrings() to tupelstore_puttuple() which is
dblink does in materializeResult(), and of cource C-string is the
most natural format in C program, and I have hesitated to modify
execTuples.c, and I wanted to hide the details of PGresAttValue.

Assuming that the values are passed as PGresAttValue* is given
(for the reasons of performance and the extent of the
modification), the adding tuple functions should get the value
from the struct. This can be done in two ways from the view of
authority (`encapsulation', in other words) and convenience, one
is with the knowledge of the structure, and the other is without
it. PQgetAsCstring is the latter approach. (And it is
inconsistent with the fact that the definition of PGresAttValue
is moved into lipq-fe.h from libpq-int.h. The details of the
structure should be hidden like PGresult in this approach).

But it is not obvious that the choice is better than the another
one. If we consider that PGresAttValue is too simple and stable
to hide the details, PQgetAsCString will be taken off and the
problem will go out. PGresAttValue needs documentation in this
case.

I prefer to handle PGresAttValue directly if no problem.

 I also don't think the add tuple terminology is particularly good.
 It's not obvious from the name that what you're doing is overriding
 the way memory is allocated and results are stored.

This phrase is taken from pqAddTuple() in fe-exec.c at first and
have not been changed after the function is integrated with other
functions.

I propose tuple storage handler for the alternative.

- typedef void *(*addTupleFunction)(...);
+ typedef void *(*tupleStorageHandler)(...);

- typedef enum { ADDTUP_*, } AddTupFunc;
+ typedef enum { TSHANDLER_*, } TSHandlerCommand;

- void *PQgetAddTupleParam(...);
+ void *PQgetTupleStrageHandlerContext(...);

- void PQregisterTupleAdder(...);
+ void PQregisterTupleStoreHandler(...);

- addTupleFunction PGresult.addTupleFunc;
+ tupleStorageHandler PGresult.tupleStorageHandlerFunc;

- void *PGresult.addTuleFuncParam;
+ void *PGresult.tupleStorageHandlerContext;

- char *PGresult.addTuleFuncErrMes;
+ void *PGresult.tupelStrageHandlerErrMes;

 Also, what about the problem Tom mentioned here?
 
 http://archives.postgresql.org/message-id/1042.1321123...@sss.pgh.pa.us

The plan that simply replace malloc's with something like
palloc's is abandoned for the narrow scope.

dblink-plus copies whole PGresult into TupleStore in order to
avoid making orphaned memory on SIGINT. The resource owner
mechanism is principally applicable to that but practically hard
for the reason that current implementation without radically
modification couldn't accept it.. In addition to that, dblink
also does same thing for maybe the same reason with dblink-plus
and another reason as far as I heard.

Whatever the reason is, both dblink and dblink-plus do the same
thing that could lower the performance than expected.

If TupleStore(TupleDesc) is preferable to PGresult for in-backend
use and oridinary(client-use) libpq users can handle only
PGresult, the mechanism like this patch would be reuired to
maintain the compatibility, I think. To the contrary, if there is
no significant reason to use TupleStore in backend use - it
implies that existing mechanisms like resource owner can save the
backend inexpensively from possible inconvenience caused by using
PGresult storage in backends - PGresult should be used as it is.

I think TupleStore prepared to be used in backend is preferable
for the usage and don't want to get data making detour via
PGresult.


regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

-- 
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] Allow substitute allocators for PGresult.

2011-12-21 Thread Robert Haas
On Thu, Dec 8, 2011 at 5:41 AM, Kyotaro HORIGUCHI
horiguchi.kyot...@oss.ntt.co.jp wrote:
  This is the patch to add the documentation of PGresult custom
  storage. It shows in section '31.19. Alternative result
  storage'.

It would be good to consolidate this into the main patch.

I find the names of the functions added here to be quite confusing and
would suggest renaming them.  I expected PQgetAsCstring to do
something similar to PQgetvalue, but the code is completely different,
and even after reading the documentation I still don't understand what
that function is supposed to be used for.  Why as cstring?  What
would the other option be?

I also don't think the add tuple terminology is particularly good.
It's not obvious from the name that what you're doing is overriding
the way memory is allocated and results are stored.

Also, what about the problem Tom mentioned here?

http://archives.postgresql.org/message-id/1042.1321123...@sss.pgh.pa.us

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
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] Allow substitute allocators for PGresult.

2011-12-17 Thread Tom Lane
Greg Smith g...@2ndquadrant.com writes:
 On 12/01/2011 05:48 AM, Kyotaro HORIGUCHI wrote:
 xfer timePeak RSS
 Original : 6.02s 850MB
 libpq patch + Original dblink: 6.11s 850MB
 full patch   : 4.44s 643MB

 These look like interesting results.  Currently Tom is listed as the 
 reviewer on this patch, based on comments made before the CF really 
 started.  And the patch has been incorrectly been sitting in Waiting 
 for author for the last week; oops.  I'm not sure what to do with this 
 one now except raise a general call to see if anyone wants to take a 
 look at it, now that it seems to be in good enough shape to deliver 
 measurable results.

I did list myself as reviewer some time ago, but if anyone else wants to
take it I won't be offended ;-)

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] Allow substitute allocators for PGresult.

2011-12-16 Thread Greg Smith

On 12/01/2011 05:48 AM, Kyotaro HORIGUCHI wrote:

xfer time   Peak RSS
Original: 6.02s 850MB
libpq patch + Original dblink   : 6.11s 850MB
full patch  : 4.44s 643MB
   


These look like interesting results.  Currently Tom is listed as the 
reviewer on this patch, based on comments made before the CF really 
started.  And the patch has been incorrectly been sitting in Waiting 
for author for the last week; oops.  I'm not sure what to do with this 
one now except raise a general call to see if anyone wants to take a 
look at it, now that it seems to be in good enough shape to deliver 
measurable results.


--
Greg Smith   2ndQuadrant USg...@2ndquadrant.com   Baltimore, MD
PostgreSQL Training, Services, and 24x7 Support  www.2ndQuadrant.us


--
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] Allow substitute allocators for PGresult.

2011-12-08 Thread Kyotaro HORIGUCHI
Hello,

 The documentation had slipped my mind.

 This is the patch to add the documentation of PGresult custom
 storage. It shows in section '31.19. Alternative result
 storage'.

 regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml
index 252ff8c..dc2acb6 100644
--- a/doc/src/sgml/libpq.sgml
+++ b/doc/src/sgml/libpq.sgml
@@ -7229,6 +7229,325 @@ int PQisthreadsafe();
  /sect1
 
 
+ sect1 id=libpq-alterstorage
+  titleAlternative result storage/title
+
+  indexterm zone=libpq-alterstorage
+   primaryPGresult/primary
+   secondaryPGconn/secondary
+  /indexterm
+
+  para
+   As the standard usage, users can get the result of command
+   execution from structnamePGresult/structname aquired
+   with functionPGgetResult/function
+   from structnamePGConn/structname. While the memory areas for
+   the PGresult are allocated with malloc() internally within calls of
+   command execution functions such as functionPQexec/function
+   and functionPQgetResult/function. If you have difficulties to
+   handle the result records in the form of PGresult, you can instruct
+   PGconn to store them into your own storage instead of PGresult.
+  /para
+
+  variablelist
+   varlistentry id=libpq-registertupleadder
+term
+ functionPQregisterTupleAdder/function
+ indexterm
+  primaryPQregisterTupleAdder/primary
+ /indexterm
+/term
+
+listitem
+ para
+   Sets a function to allocate memory for each tuple and column
+   values, and add the completed tuple into your storage.
+synopsis
+void PQregisterTupleAdder(PGconn *conn,
+  addTupleFunction func,
+  void *param);
+/synopsis
+ /para
+ 
+ para
+   variablelist
+	 varlistentry
+	   termparameterconn/parameter/term
+	   listitem
+	 para
+	   The connection object to set the tuple adder
+	   function. PGresult created from this connection calles
+	   this function to store the result tuples instead of
+	   storing into its internal storage.
+	 /para
+	   /listitem
+	 /varlistentry
+	 varlistentry
+	   termparameterfunc/parameter/term
+	   listitem
+	 para
+	   Tuple adder function to set. NULL means to use the
+	   default storage.
+	 /para
+	   /listitem
+	 /varlistentry
+	 varlistentry
+	   termparameterparam/parameter/term
+	   listitem
+	 para
+	   A pointer to contextual parameter passed
+	   to parameterfunc/parameter.
+	 /para
+	   /listitem
+	 /varlistentry
+   /variablelist
+ /para
+/listitem
+   /varlistentry
+  /variablelist
+
+  variablelist
+   varlistentry id=libpq-addtuplefunction
+term
+ typeaddTupleFunction/type
+ indexterm
+  primaryaddTupleFunction/primary
+ /indexterm
+/term
+
+listitem
+ para
+   The type for the callback function to serve memory blocks for
+   each tuple and its column values, and to add the constructed
+   tuple into your own storage.
+synopsis
+typedef enum 
+{
+  ADDTUP_ALLOC_TEXT,
+  ADDTUP_ALLOC_BINARY,
+  ADDTUP_ADD_TUPLE
+} AddTupFunc;
+
+void *(*addTupleFunction)(PGresult *res,
+  AddTupFunc func,
+  int id,
+  size_t size);
+/synopsis
+ /para
+
+ para
+   Generally this function must return NULL for failure and should
+   set the error message
+   with functionPGsetAddTupleErrMes/function if the cause is
+   other than out of memory. This funcion must not throw any
+   exception. This function is called in the sequence following.
+
+   itemizedlist spacing=compact
+	 listitem
+	   simparaCall with parameterfunc/parameter
+	   = firsttermADDTUP_ALLOC_BINARY/firstterm
+	   and parameterid/parameter = -1 to request the memory
+	   for tuple used as an array
+	   of typePGresAttValue/type /simpara
+	 /listitem
+	 listitem
+	   simparaCall with parameterfunc/parameter
+	   = firsttermADDTUP_ALLOC_TEXT/firstterm
+	   or firsttermADDTUP_ALLOC_TEXT/firstterm
+	   and parameterid/parameter is zero or positive number
+	   to request the memory for each column value in current
+	   tuple./simpara
+	 /listitem
+	 listitem
+	   simparaCall with parameterfunc/parameter
+	   = firsttermADDTUP_ADD_TUPLE/firstterm to request the
+	   constructed tuple to store./simpara
+	 /listitem
+   /itemizedlist
+ /para
+ para
+   Calling typeaddTupleFunction/type
+   with parameterfunc/parameter =
+   firsttermADDTUP_ALLOC_TEXT/firstterm is telling to return a
+memory block with at least parametersize/parameter bytes
+which may not be aligned to the word boundary.
+   parameterid/parameter is a zero or positive number
+   distinguishes the usage of requested memory block, that is the
+   position of the column for which the memory block is used.
+ /para
+ para
+   When parameterfunc/parameter
+   = 

Re: [HACKERS] Allow substitute allocators for PGresult.

2011-12-01 Thread Kyotaro HORIGUCHI
Hello, 

me I'll put further thought into dblink-plus taking it into
me account.
..
me Please let me have a little more time.

I've inquired the developer of dblink-plus about
RegisterResourceReleaseCallback(). He said that the function is
in bad compatibility with current implementation. In addition to
this, storing into tuplestore directly seems to me a good idea
than palloc'ed PGresult.

So I tried to make libpq/PGresult be able to handle alternative
tuple store by hinting to PGconn, and modify dblink to use the
mechanism as the first sample code.

I will show it as a series of patches in next message.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

-- 
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] Allow substitute allocators for PGresult.

2011-12-01 Thread Kyotaro HORIGUCHI
Hello, This is the next version of Allow substitute allocators
for PGresult.

Totally chaning the concept from the previous one, this patch
allows libpq to handle alternative tuple store for received
tuples.

Design guidelines are shown below.

 - No need to modify existing client code of libpq.

 - Existing libpq client runs with roughly same performance, and
   dblink with modification runs faster to some extent and
   requires less memory.

I have measured roughly of run time and memory requirement for
three configurations on CentOS6 on Vbox with 2GB mem 4 cores
running on Win7-Corei7, transferring (30 bytes * 2 cols) *
200 tuples (120MB net) within this virutal machine. The
results are below.

  xfer time Peak RSS
Original: 6.02s 850MB
libpq patch + Original dblink   : 6.11s 850MB
full patch  : 4.44s 643MB

xfer time here is the mean of five 'real time's measured by
running sql script like this after warmup run.

=== test.sql
select dblink_connect('c', 'host=localhost port=5432 dbname=test');
select * from dblink('c', 'select a,c from foo limit 200') as (a text, b 
bytea) limit 1;

select dblink_disconnect('c');
===
$  for i in $(seq 1 10); do time psql test -f t.sql; done 
===

Peak RSS is measured by picking up heap Rss in /proc/[pid]/smaps.


It seems somewhat slow using patched libpq and original dblink,
but it seems within error range too. If this amount of slowdown
is not permissible, it might be improved by restoring the static
call route before for extra redundancy of the code.

On the other hand, full patch version seems obviously fast and
requires less memory. Isn't it nice?

This patch consists of two sub patches.

The first is a patch for libpq to allow rewiring tuple storage
mechanism. But default behavior is not changed. Existing libpq
client should run with it.

The second is modify dblink to storing received tuples into
tuplestore directly using the mechanism above.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index a360d78..1af8df6 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -160,7 +160,3 @@ PQconnectStartParams  157
 PQping158
 PQpingParams  159
 PQlibVersion  160
-PQregisterTupleAdder	  161
-PQgetAsCstring		  162
-PQgetAddTupleParam	  163
-PQsetAddTupleErrMes	  164
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 437be26..50f3f83 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2692,7 +2692,6 @@ makeEmptyPGconn(void)
 	conn-allow_ssl_try = true;
 	conn-wait_ssl_try = false;
 #endif
-	conn-addTupleFunc = NULL;
 
 	/*
 	 * We try to send at least 8K at a time, which is the usual size of pipe
@@ -5065,10 +5064,3 @@ PQregisterThreadLock(pgthreadlock_t newhandler)
 
 	return prev;
 }
-
-void
-PQregisterTupleAdder(PGconn *conn, addTupleFunction func, void *param)
-{
-	conn-addTupleFunc = func;
-	conn-addTupleFuncParam = param;
-}
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index c8ec9bd..113aab0 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -48,6 +48,7 @@ char	   *const pgresStatus[] = {
 static int	static_client_encoding = PG_SQL_ASCII;
 static bool static_std_strings = false;
 
+
 static PGEvent *dupEvents(PGEvent *events, int count);
 static bool PQsendQueryStart(PGconn *conn);
 static int PQsendQueryGuts(PGconn *conn,
@@ -65,9 +66,7 @@ static PGresult *PQexecFinish(PGconn *conn);
 static int PQsendDescribe(PGconn *conn, char desc_type,
 			   const char *desc_target);
 static int	check_field_number(const PGresult *res, int field_num);
-static void *pqDefaultAddTupleFunc(PGresult *res, AddTupFunc func,
-   int id, size_t len);
-static void *pqAddTuple(PGresult *res, PGresAttValue *tup);
+
 
 /* 
  * Space management for PGresult.
@@ -161,9 +160,6 @@ PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status)
 	result-curBlock = NULL;
 	result-curOffset = 0;
 	result-spaceLeft = 0;
-	result-addTupleFunc = pqDefaultAddTupleFunc;
-	result-addTupleFuncParam = NULL;
-	result-addTupleFuncErrMes = NULL;
 
 	if (conn)
 	{
@@ -198,12 +194,6 @@ PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status)
 			}
 			result-nEvents = conn-nEvents;
 		}
-
-		if (conn-addTupleFunc)
-		{
-			result-addTupleFunc = conn-addTupleFunc;
-			result-addTupleFuncParam = conn-addTupleFuncParam;
-		}
 	}
 	else
 	{
@@ -497,33 +487,6 @@ PQresultAlloc(PGresult *res, size_t nBytes)
 	return pqResultAlloc(res, nBytes, TRUE);
 }
 
-void *
-pqDefaultAddTupleFunc(PGresult *res, AddTupFunc func, int id, size_t len)
-{
-	void *p;
-
-	switch (func)
-	{
-		case ADDTUP_ALLOC_TEXT:
-			return pqResultAlloc(res, len, TRUE);
-
-		case ADDTUP_ALLOC_BINARY:
-			

Re: [HACKERS] Allow substitute allocators for PGresult.

2011-12-01 Thread Kyotaro HORIGUCHI
Ouch! I'm sorry for making a reverse patch for the first modification.

This is an amendment of the message below. The body text is
copied into this message.

http://archives.postgresql.org/message-id/20111201.192419.103527179.horiguchi.kyot...@oss.ntt.co.jp

===
Hello, This is the next version of Allow substitute allocators
for PGresult.

Totally chaning the concept from the previous one, this patch
allows libpq to handle alternative tuple store for received
tuples.

Design guidelines are shown below.

 - No need to modify existing client code of libpq.

 - Existing libpq client runs with roughly same performance, and
   dblink with modification runs faster to some extent and
   requires less memory.

I have measured roughly of run time and memory requirement for
three configurations on CentOS6 on Vbox with 2GB mem 4 cores
running on Win7-Corei7, transferring (30 bytes * 2 cols) *
200 tuples (120MB net) within this virutal machine. The
results are below.

  xfer time Peak RSS
Original: 6.02s 850MB
libpq patch + Original dblink   : 6.11s 850MB
full patch  : 4.44s 643MB

xfer time here is the mean of five 'real time's measured by
running sql script like this after warmup run.

=== test.sql
select dblink_connect('c', 'host=localhost port=5432 dbname=test');
select * from dblink('c', 'select a,c from foo limit 200') as (a text, b 
bytea) limit 1;

select dblink_disconnect('c');
===
$  for i in $(seq 1 10); do time psql test -f t.sql; done 
===

Peak RSS is measured by picking up heap Rss in /proc/[pid]/smaps.


It seems somewhat slow using patched libpq and original dblink,
but it seems within error range too. If this amount of slowdown
is not permissible, it might be improved by restoring the static
call route before for extra redundancy of the code.

On the other hand, full patch version seems obviously fast and
requires less memory. Isn't it nice?

This patch consists of two sub patches.

The first is a patch for libpq to allow rewiring tuple storage
mechanism. But default behavior is not changed. Existing libpq
client should run with it.

The second is modify dblink to storing received tuples into
tuplestore directly using the mechanism above.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center


diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt
index 1af8df6..a360d78 100644
--- a/src/interfaces/libpq/exports.txt
+++ b/src/interfaces/libpq/exports.txt
@@ -160,3 +160,7 @@ PQconnectStartParams  157
 PQping158
 PQpingParams  159
 PQlibVersion  160
+PQregisterTupleAdder	  161
+PQgetAsCstring		  162
+PQgetAddTupleParam	  163
+PQsetAddTupleErrMes	  164
diff --git a/src/interfaces/libpq/fe-connect.c b/src/interfaces/libpq/fe-connect.c
index 50f3f83..437be26 100644
--- a/src/interfaces/libpq/fe-connect.c
+++ b/src/interfaces/libpq/fe-connect.c
@@ -2692,6 +2692,7 @@ makeEmptyPGconn(void)
 	conn-allow_ssl_try = true;
 	conn-wait_ssl_try = false;
 #endif
+	conn-addTupleFunc = NULL;
 
 	/*
 	 * We try to send at least 8K at a time, which is the usual size of pipe
@@ -5064,3 +5065,10 @@ PQregisterThreadLock(pgthreadlock_t newhandler)
 
 	return prev;
 }
+
+void
+PQregisterTupleAdder(PGconn *conn, addTupleFunction func, void *param)
+{
+	conn-addTupleFunc = func;
+	conn-addTupleFuncParam = param;
+}
diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c
index 113aab0..c8ec9bd 100644
--- a/src/interfaces/libpq/fe-exec.c
+++ b/src/interfaces/libpq/fe-exec.c
@@ -48,7 +48,6 @@ char	   *const pgresStatus[] = {
 static int	static_client_encoding = PG_SQL_ASCII;
 static bool static_std_strings = false;
 
-
 static PGEvent *dupEvents(PGEvent *events, int count);
 static bool PQsendQueryStart(PGconn *conn);
 static int PQsendQueryGuts(PGconn *conn,
@@ -66,7 +65,9 @@ static PGresult *PQexecFinish(PGconn *conn);
 static int PQsendDescribe(PGconn *conn, char desc_type,
 			   const char *desc_target);
 static int	check_field_number(const PGresult *res, int field_num);
-
+static void *pqDefaultAddTupleFunc(PGresult *res, AddTupFunc func,
+   int id, size_t len);
+static void *pqAddTuple(PGresult *res, PGresAttValue *tup);
 
 /* 
  * Space management for PGresult.
@@ -160,6 +161,9 @@ PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status)
 	result-curBlock = NULL;
 	result-curOffset = 0;
 	result-spaceLeft = 0;
+	result-addTupleFunc = pqDefaultAddTupleFunc;
+	result-addTupleFuncParam = NULL;
+	result-addTupleFuncErrMes = NULL;
 
 	if (conn)
 	{
@@ -194,6 +198,12 @@ PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status)
 			}
 			result-nEvents = conn-nEvents;
 		}
+
+		if (conn-addTupleFunc)
+		{
+			result-addTupleFunc = conn-addTupleFunc;
+			result-addTupleFuncParam = conn-addTupleFuncParam;
+		}
 	}
 	else
 	{
@@ -487,6 +497,33 @@ PQresultAlloc(PGresult *res, size_t nBytes)
 	

Re: [HACKERS] Allow substitute allocators for PGresult.

2011-11-22 Thread Kyotaro HORIGUCHI
Hello,

At Fri, 11 Nov 2011 11:29:30 +0200, Heikki Linnakangas wrote
 You could use the resource owner mechanism to track
 them. Register a callback function with
 RegisterResourceReleaseCallback().

Thank you for letting me know about it. I have dug up a message
in pg-hackers refering to the mechanism on discussion about
postgresql-fdw. I'll put further thought into dblink-plus taking
it into account.


By the way, thinking about memory management for the result in
libpq is considerable as another issue.

At Sat, 12 Nov 2011 12:29:50 -0500, Tom Lane wrote
 To start with, the patch proposes exposing some global
 variables that affect the behavior of libpq process-wide.  This
 seems like a pretty bad design, because a single process could
 contain multiple usages of libpq

You're right to say the design is bad. I've designed it to have
minimal impact on libpq by limiting usage and imposing any
reponsibility on the users, that is the developers of the modules
using it. If there are any other applications that want to use
their own allocators, there are some points to be considered.

I think it is preferable consiering multi-threading to make libpq
write PGresult into memory blocks passed from the application
like OCI does, instead of letting libpq itself make request for
them.

This approach hands the responsibility of memory management to
the user and gives them the capability to avoid memory exhaustion
by their own measures.

On the other hand, this way could produce the situation that
libpq cannot write all of the data to receive from the server
onto handed memory block. So, the API must be able to return the
partial data to the caller.

More advancing, if libpq could store the result directly into
user-allocated memory space using tuplestore-like interface, it
is better on performance if the final storage is a tuplestore
itself.

I will be happy with the part-by-part passing of result. So I
will think about this as the next issue.


 So I'd feel happier about the patch if it came along with a
 patch to make dblink.c use it to prevent memory leaks.

I take it is about my original patch.

Mmm, I heard that dblink copies received data in PGResult to
tuple store not only because of the memory leak, but less memory
usage (after the copy is finished). I think I could show you the
patch ignoring the latter, but it might take some time for me to
start from understand dblink and tuplestore closely...


If I find RegisterResourceReleaseCallback short for our
requirement, I will show it. If not, I withdraw this patch for
ongoing CF and propose another patch based on the discussion
above at another time.


Please let me have a little more time.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

-- 
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] Allow substitute allocators for PGresult.

2011-11-12 Thread Tom Lane
Kyotaro HORIGUCHI horiguchi.kyot...@oss.ntt.co.jp writes:
 Hello. This message is a proposal of a pair of patches that
 enables the memory allocator for PGresult in libpq to be
 replaced.

Since there seems to be rough consensus that something like this would
be a good idea, I looked more closely at the details of the patch.
I think the design could use some adjustment.

To start with, the patch proposes exposing some global variables that
affect the behavior of libpq process-wide.  This seems like a pretty bad
design, because a single process could contain multiple usages of libpq
with different requirements.  As an example, if dblink.c were to set
these variables inside a backend process, it would break usage of libpq
from PL/Perl via DBI.  Global variables also tend to be a bad idea
whenever you think about multi-threaded applications --- they require
locking facilities, which are not in this patch.

I think it'd be better to consider the PGresult alloc/free functions to
be a property of a PGconn, which you'd set with a function call along the
lines of PQsetResultAllocator(conn, alloc_func, realloc_func, free_func)
after having successfully opened a connection.  Then we just have some
more PGconn fields (and I guess PGresult will need a copy of the
free_func pointer) and no new global variables.

I am also feeling dubious about whether it's a good idea to expect the
functions to have exactly the signature of malloc/free.  They are
essentially callbacks, and in most places where a library provides for
callbacks, it's customary to include a void * passthrough argument
in case the callback needs some context information.  I am not sure that
dblink.c would need such a thing, but if we're trying to design a
general-purpose feature, then we probably should have it.  The cost
would be having shim functions inside libpq for the default case, but
it doesn't seem likely that they'd cost enough to notice.

The patch lacks any user documentation, which it surely must have if
we are claiming this is a user-visible feature.  And I think it could
use some attention to updating code comments, notably the large block
about PGresult space management near the top of fe-exec.c.

Usually, when writing a feature of this sort, it's a good idea to
implement a prototype use-case to make sure you've not overlooked
anything.  So I'd feel happier about the patch if it came along with
a patch to make dblink.c use it to prevent memory leaks.

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] Allow substitute allocators for PGresult.

2011-11-12 Thread Tom Lane
Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes:
 On 11.11.2011 11:18, Kyotaro HORIGUCHI wrote:
 For these reasons, I propose to make allocators for PGresult
 replaceable.

 You could use the resource owner mechanism to track them.

BTW, I just thought of a potentially fatal objection to making PGresult
allocation depend on palloc: libpq is absolutely not prepared to handle
losing control on out-of-memory.  While I'm not certain that its
behavior with malloc is entirely desirable either (it tends to loop in
hopes of getting the memory next time), we cannot just plop in palloc
in place of malloc and imagine that we're not breaking it.

This makes me think that Heikki's approach is by far the more tenable
one, so far as dblink is concerned.  Perhaps the substitute-malloc idea
is still useful for some other application, but I'm inclined to put that
idea on the back burner until we have a concrete use case for it.

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] Allow substitute allocators for PGresult.

2011-11-12 Thread Matteo Beccati

On 12/11/2011 07:36, Robert Haas wrote:

On Sat, Nov 12, 2011 at 12:48 AM, Tom Lanet...@sss.pgh.pa.us  wrote:

AIUI Kyotaro-san is just suggesting that the app should be able to
provide a substitute malloc function for use in allocating PGresult
space (and not, I think, anything else that libpq allocates internally).
Basically this would allow PGresults to be cleaned up with methods other
than calling PQclear on each one.  It wouldn't affect how you'd interact
with one while you had it.  That seems like pretty much exactly what we
want for preventing memory leaks in the backend; but is it going to be
useful for other apps?


I think it will.


Maybe I've just talking nonsense, I just have little experience hacking 
the pgsql and pdo-pgsql exstensions, but to me it would seem something 
that could easily avoid an extra duplication of the data returned by 
pqgetvalue. To me it seems a pretty nice win.



Cheers
--
Matteo Beccati

Development  Consulting - http://www.beccati.com/

--
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] Allow substitute allocators for PGresult.

2011-11-12 Thread Stephen Frost
* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Well, loading data in a form whereby the application can access it
 without going through the PGresult accessor functions would be an
 entirely different (and vastly larger) project.  

Looking through the thread, I agree that it's a different thing than
what's being discussed here.

 I'm not sure I want
 to open that can of worms --- it seems like you could write a huge
 amount of code trying to provide every format someone might want,
 and still find that there were impedance mismatches for many
 applications.

The OCI approach is actually very similar to how we handle our
catalogs internally..  Imagine you define a C struct which matched your
table structure, then you allocate 5000 (or however) of those, give the
base pointer to the 'getResult' call and a integer array of offsets into
that structure for each of the columns.  There might have been a few
other minor things (like some notion of how to handle NULLs), but it was
pretty straight-forward from the C perspective, imv.

Trying to provide alternative formats (I'm guessing you were referring
to something like XML..?  Or some complex structure?) would certainly be
a whole different ballgame.

Thanks,

Stephen

 AIUI Kyotaro-san is just suggesting that the app should be able to
 provide a substitute malloc function for use in allocating PGresult
 space (and not, I think, anything else that libpq allocates internally).
 Basically this would allow PGresults to be cleaned up with methods other
 than calling PQclear on each one.  It wouldn't affect how you'd interact
 with one while you had it.  That seems like pretty much exactly what we
 want for preventing memory leaks in the backend; but is it going to be
 useful for other apps?
 
   regards, tom lane


signature.asc
Description: Digital signature


Re: [HACKERS] Allow substitute allocators for PGresult.

2011-11-11 Thread Heikki Linnakangas

On 11.11.2011 11:18, Kyotaro HORIGUCHI wrote:

The comment at the the begging of pqexpbuffer.c says that libpq
should not rely on palloc(). Besides, Tom Lane said that palloc
should not be visible outside the backend(*1) and I agree with
it.

*1: http://archives.postgresql.org/pgsql-hackers/1999-02/msg00364.php

On the other hand, in dblink, dblink-plus (our product!), and
maybe FDW's connect to other PostgreSQL servers are seem to copy
the result of the query contained in PGresult into tuple store. I
guess that this is in order to avoid memory leakage on
termination in halfway.

But it is rather expensive to copy whole PGresult, and the
significance grows as the data received gets larger. Furthermore,
it requires about twice as much memory as the net size of the
data. And it is fruitless to copy'n modify libpq or reinvent it
from scratch. So we shall be happy to be able to use palloc's in
libpq at least for PGresult for such case in spite of the policy.


For these reasons, I propose to make allocators for PGresult
replaceable.


You could use the resource owner mechanism to track them. Register a 
callback function with RegisterResourceReleaseCallback(). Whenever a 
PGresult is returned from libpq, add it to e.g a linked list, kept in 
TopMemoryContext, and also store a reference to CurrentResourceOwner in 
the list element. In the callback function, scan through the list and 
free all the PGresults associated with the resource owner that's being 
released.


--
  Heikki Linnakangas
  EnterpriseDB   http://www.enterprisedb.com

--
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] Allow substitute allocators for PGresult.

2011-11-11 Thread Tom Lane
Heikki Linnakangas heikki.linnakan...@enterprisedb.com writes:
 On 11.11.2011 11:18, Kyotaro HORIGUCHI wrote:
 The comment at the the begging of pqexpbuffer.c says that libpq
 should not rely on palloc(). Besides, Tom Lane said that palloc
 should not be visible outside the backend(*1) and I agree with
 it.
 
 *1: http://archives.postgresql.org/pgsql-hackers/1999-02/msg00364.php
 
 On the other hand, in dblink, dblink-plus (our product!), and
 maybe FDW's connect to other PostgreSQL servers are seem to copy
 the result of the query contained in PGresult into tuple store. I
 guess that this is in order to avoid memory leakage on
 termination in halfway.
 
 But it is rather expensive to copy whole PGresult, and the
 significance grows as the data received gets larger. Furthermore,
 it requires about twice as much memory as the net size of the
 data. And it is fruitless to copy'n modify libpq or reinvent it
 from scratch. So we shall be happy to be able to use palloc's in
 libpq at least for PGresult for such case in spite of the policy.
 
 For these reasons, I propose to make allocators for PGresult
 replaceable.

 You could use the resource owner mechanism to track them.

Heikki's idea is probably superior so far as PG backend usage is
concerned in isolation, but I wonder if there are scenarios where a
client application would like to be able to manage libpq's allocations.
If so, Kyotaro-san's approach would solve more problems than just
dblink's.

However, the bigger picture here is that I think Kyotaro-san's desire to
not have dblink return a tuplestore may be misplaced.  Tuplestores can
spill to disk, while PGresults don't; so the larger the result, the
more important it is to push it into a tuplestore and PQclear it as soon
as possible.

Despite that worry, it'd likely be a good idea to adopt one or the other
of these solutions anyway, because I think there are corner cases where
dblink.c can leak a PGresult --- for instance, what if dblink_res_error
fails due to out-of-memory before reaching PQclear?  And we could get
rid of the awkward and none-too-cheap PG_TRY blocks that it uses to try
to defend against such leaks in other places.

So I'm in favor of making a change along that line, although I'd want
to see more evidence before considering changing dblink to not return
tuplestores.

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] Allow substitute allocators for PGresult.

2011-11-11 Thread Stephen Frost
* Tom Lane (t...@sss.pgh.pa.us) wrote:
 Heikki's idea is probably superior so far as PG backend usage is
 concerned in isolation, but I wonder if there are scenarios where a
 client application would like to be able to manage libpq's allocations.

The answer to that is certainly 'yes'.  It was one of the first things
that I complained about when moving from Oracle to PG.  With OCI, you
can bulk load results directly into application-allocated memory areas.

Haven't been following the dblink discussion, so not going to comment
about that piece.

Thanks,

Stephen


signature.asc
Description: Digital signature


Re: [HACKERS] Allow substitute allocators for PGresult.

2011-11-11 Thread Tom Lane
Stephen Frost sfr...@snowman.net writes:
 * Tom Lane (t...@sss.pgh.pa.us) wrote:
 Heikki's idea is probably superior so far as PG backend usage is
 concerned in isolation, but I wonder if there are scenarios where a
 client application would like to be able to manage libpq's allocations.

 The answer to that is certainly 'yes'.  It was one of the first things
 that I complained about when moving from Oracle to PG.  With OCI, you
 can bulk load results directly into application-allocated memory areas.

Well, loading data in a form whereby the application can access it
without going through the PGresult accessor functions would be an
entirely different (and vastly larger) project.  I'm not sure I want
to open that can of worms --- it seems like you could write a huge
amount of code trying to provide every format someone might want,
and still find that there were impedance mismatches for many
applications.

AIUI Kyotaro-san is just suggesting that the app should be able to
provide a substitute malloc function for use in allocating PGresult
space (and not, I think, anything else that libpq allocates internally).
Basically this would allow PGresults to be cleaned up with methods other
than calling PQclear on each one.  It wouldn't affect how you'd interact
with one while you had it.  That seems like pretty much exactly what we
want for preventing memory leaks in the backend; but is it going to be
useful for other apps?

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] Allow substitute allocators for PGresult.

2011-11-11 Thread Robert Haas
On Sat, Nov 12, 2011 at 12:48 AM, Tom Lane t...@sss.pgh.pa.us wrote:
 AIUI Kyotaro-san is just suggesting that the app should be able to
 provide a substitute malloc function for use in allocating PGresult
 space (and not, I think, anything else that libpq allocates internally).
 Basically this would allow PGresults to be cleaned up with methods other
 than calling PQclear on each one.  It wouldn't affect how you'd interact
 with one while you had it.  That seems like pretty much exactly what we
 want for preventing memory leaks in the backend; but is it going to be
 useful for other apps?

I think it will.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers