Re: [HACKERS] Allow substitute allocators for PGresult.
Hello, thank you for taking the time for comment.
At Wed, 21 Dec 2011 11:09:59 -0500, Robert Haas 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.
On Thu, Dec 8, 2011 at 5:41 AM, Kyotaro HORIGUCHI 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.
Greg Smith 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.
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.
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();
+
+ Alternative result storage
+
+
+ PGresult
+ PGconn
+
+
+
+ As the standard usage, users can get the result of command
+ execution from PGresult aquired
+ with PGgetResult
+ from PGConn. While the memory areas for
+ the PGresult are allocated with malloc() internally within calls of
+ command execution functions such as PQexec
+ and PQgetResult. 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.
+
+
+
+
+
+ PQregisterTupleAdder
+
+ PQregisterTupleAdder
+
+
+
+
+
+ Sets a function to allocate memory for each tuple and column
+ values, and add the completed tuple into your storage.
+
+void PQregisterTupleAdder(PGconn *conn,
+ addTupleFunction func,
+ void *param);
+
+
+
+
+
+
+ conn
+
+
+ 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.
+
+
+
+
+ func
+
+
+ Tuple adder function to set. NULL means to use the
+ default storage.
+
+
+
+
+ param
+
+
+ A pointer to contextual parameter passed
+ to func.
+
+
+
+
+
+
+
+
+
+
+
+
+ addTupleFunction
+
+ addTupleFunction
+
+
+
+
+
+ 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.
+
+typedef enum
+{
+ ADDTUP_ALLOC_TEXT,
+ ADDTUP_ALLOC_BINARY,
+ ADDTUP_ADD_TUPLE
+} AddTupFunc;
+
+void *(*addTupleFunction)(PGresult *res,
+ AddTupFunc func,
+ int id,
+ size_t size);
+
+
+
+
+ Generally this function must return NULL for failure and should
+ set the error message
+ with PGsetAddTupleErrMes if the cause is
+ other than out of memory. This funcion must not throw any
+ exception. This function is called in the sequence following.
+
+
+
+ Call with func
+ = ADDTUP_ALLOC_BINARY
+ and id = -1 to request the memory
+ for tuple used as an array
+ of PGresAttValue
+
+
+ Call with func
+ = ADDTUP_ALLOC_TEXT
+ or ADDTUP_ALLOC_TEXT
+ and id is zero or positive number
+ to request the memory for each column value in current
+ tuple.
+
+
+ Call with func
+ = ADDTUP_ADD_TUPLE to request the
+ constructed tuple to store.
+
+
+
+
+ Calling addTupleFunction
+ with func =
+ ADDTUP_ALLOC_TEXT is telling to return a
+memory block with at least size bytes
+which may not be aligned to the word boundary.
+ id 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.
+
+
+ When func
+ = ADDTUP_ALLOC_BINARY, this function is
+ telled to return a memory block with at
+ least size bytes which is aligned to the
+ word boundary.
+ id is the identifier distinguishes the
+ usage of requested memory block. -1 means that it is used as an
+ array of PGresAttValue to store the tuple. Zero or
+ positive numbers have the same meanings as for
+ ADDTUP_ALLOC_BINARY.
+
+ When func
+ = ADDTUP_ADD_TUPLE, this function is
+ telled to store the PGresAttValue structure
+ constructed by the caller into your storage. The pointer to the
+ tuple structure is not passed so you should memorize the
+ pointer to the memory block passed the caller on
+ func
+ = ADDTUP_ALLOC_BINARY
+ with id is -1. This function must return
+ any non-NULL values for success. You must properly put back the
+ memory blocks passed to the caller for this function if needed.
+
+
+
+ res
+
+
+ A pointer to the PGresult object.
+
+
+
+
+ func
+
+
+ An enum value telling the function to perform.
+
+
+
+
+ param
+
+
+ A pointer to contextual parameter passed to func.
+
+
+
+
+
+
+
+
+
+
Re: [HACKERS] Allow substitute allocators for PGresult.
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,
Re: [HACKERS] Allow substitute allocators for PGresult.
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_A
Re: [HACKERS] Allow substitute allocators for PGresult.
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.
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.
* 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.
On 12/11/2011 07:36, Robert Haas wrote: On Sat, Nov 12, 2011 at 12:48 AM, Tom Lane 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.
Heikki Linnakangas 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.
Kyotaro HORIGUCHI 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.
On Sat, Nov 12, 2011 at 12:48 AM, Tom Lane 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
Re: [HACKERS] Allow substitute allocators for PGresult.
Stephen Frost 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.
* 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.
Heikki Linnakangas 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.
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
