Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-05 Thread Marko Kreen
On Wed, Apr 04, 2012 at 06:41:00PM -0400, Tom Lane wrote: Marko Kreen mark...@gmail.com writes: On Wed, Apr 4, 2012 at 10:17 PM, Tom Lane t...@sss.pgh.pa.us wrote: Given the lack of consensus around the suspension API, maybe the best way to get the underlying libpq patch to a committable

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-05 Thread Tom Lane
Marko Kreen mark...@gmail.com writes: Minor cleanups: * Change callback return value to be 'bool': 0 is error. Currently the accepted return codes are 1 and -1, which is weird. No, I did it that way intentionally, with the thought that we might add back return code zero (or other return

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-05 Thread Marko Kreen
On Thu, Apr 05, 2012 at 12:49:37PM -0400, Tom Lane wrote: Marko Kreen mark...@gmail.com writes: Minor cleanups: * Change callback return value to be 'bool': 0 is error. Currently the accepted return codes are 1 and -1, which is weird. No, I did it that way intentionally, with the

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-04 Thread Tom Lane
Marko Kreen mark...@gmail.com writes: On Tue, Apr 03, 2012 at 05:32:25PM -0400, Tom Lane wrote: Well, there are really four levels to the API design: * Plain old PQexec. * Break down PQexec into PQsendQuery and PQgetResult. * Avoid waiting in PQgetResult by testing PQisBusy. * Avoid waiting

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-04 Thread Kyotaro HORIGUCHI
Hello, This is the new version of dblink patch. - Calling dblink_is_busy prevents row processor from being used. - some PGresult leak fixed. - Rebased to current head. A hack on top of that hack would be to collect the data into a tuplestore that contains all text columns, and then convert

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-04 Thread Tom Lane
Kyotaro HORIGUCHI horiguchi.kyot...@oss.ntt.co.jp writes: What I'm currently thinking we should do is just use the old method for async queries, and only optimize the synchronous case. Ok, I agree with you except for performance issue. I give up to use row processor for async query with

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-04 Thread Marko Kreen
On Wed, Apr 4, 2012 at 10:17 PM, Tom Lane t...@sss.pgh.pa.us wrote: Given the lack of consensus around the suspension API, maybe the best way to get the underlying libpq patch to a committable state is to take it out --- that is, remove the return zero option for row processors. Since we don't

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-04 Thread Kyotaro HORIGUCHI
I'm afraid not re-initializing materialize_needed for the next query in the latest dblink patch. I will confirm that and send the another one if needed in a few hours. # I need to catch the train I usually get on.. Hello, This is the new version of dblink patch. regards, -- Kyotaro Horiguchi

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-04 Thread Tom Lane
Marko Kreen mark...@gmail.com writes: On Wed, Apr 4, 2012 at 10:17 PM, Tom Lane t...@sss.pgh.pa.us wrote: Given the lack of consensus around the suspension API, maybe the best way to get the underlying libpq patch to a committable state is to take it out --- that is, remove the return zero

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-04 Thread Tom Lane
Kyotaro HORIGUCHI horiguchi.kyot...@oss.ntt.co.jp writes: I'm afraid not re-initializing materialize_needed for the next query in the latest dblink patch. I will confirm that and send the another one if needed in a few hours. I've committed a revised version of the previous patch. I'm not

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-04 Thread Kyotaro HORIGUCHI
I'm afraid not re-initializing materialize_needed for the next query in the latest dblink patch. I've found no need to worry about the re-initializing issue. I've committed a revised version of the previous patch. Thank you for that. I'm not sure that the case of dblink_is_busy not

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-03 Thread Marko Kreen
On Sun, Apr 01, 2012 at 07:23:06PM -0400, Tom Lane wrote: Marko Kreen mark...@gmail.com writes: So the proper approach would be to have new API call, designed to handle it, and allow early-exit only from there. That would also avoid any breakage of old APIs. Also it would avoid any

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-03 Thread Tom Lane
Marko Kreen mark...@gmail.com writes: The fact remains that upper-level code must cooperate with callback. Why is it useful to hijack PQgetResult() to do so? Because that's the defined communications channel. We're not hijacking it. If we're going to start using pejorative words here, I will

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-03 Thread Marko Kreen
On Tue, Apr 03, 2012 at 05:32:25PM -0400, Tom Lane wrote: Marko Kreen mark...@gmail.com writes: The fact remains that upper-level code must cooperate with callback. Why is it useful to hijack PQgetResult() to do so? Because that's the defined communications channel. We're not hijacking

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-01 Thread Tom Lane
I've been thinking some more about the early-termination cases (where the row processor returns zero or longjmps), and I believe I have got proposals that smooth off most of the rough edges there. First off, returning zero is really pretty unsafe as it stands, because it only works

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-01 Thread Marko Kreen
On Sun, Apr 01, 2012 at 05:51:19PM -0400, Tom Lane wrote: I've been thinking some more about the early-termination cases (where the row processor returns zero or longjmps), and I believe I have got proposals that smooth off most of the rough edges there. First off, returning zero is really

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-04-01 Thread Tom Lane
Marko Kreen mark...@gmail.com writes: Seems we both lost sight of actual usage scenario for the early-exit logic - that both callback and upper-level code *must* cooperate for it to be useful. Instead, we designed API for non-cooperating case, which is wrong. Exactly. So you need an extra

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-31 Thread Tom Lane
Marko Kreen mark...@gmail.com writes: On Thu, Mar 29, 2012 at 06:56:30PM -0400, Tom Lane wrote: Yeah. Perhaps we should tweak the row-processor callback API so that it gets an explicit notification that this is a new resultset. Duplicating PQexec's behavior would then involve having the

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-30 Thread Marko Kreen
On Thu, Mar 29, 2012 at 06:56:30PM -0400, Tom Lane wrote: Marko Kreen mark...@gmail.com writes: My conclusion is that row-processor API is low-level expert API and quite easy to misuse. It would be preferable to have something more robust as end-user API, the PQgetRow() is my suggestion

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-30 Thread Tom Lane
Marko Kreen mark...@gmail.com writes: On Thu, Mar 29, 2012 at 06:56:30PM -0400, Tom Lane wrote: Marko Kreen mark...@gmail.com writes: Second conclusion is that current dblink row-processor usage is broken when user uses multiple SELECTs in SQL as dblink uses plain PQexec(). Yeah. Perhaps we

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-30 Thread Marko Kreen
On Fri, Mar 30, 2012 at 11:59:12AM -0400, Tom Lane wrote: Marko Kreen mark...@gmail.com writes: On Thu, Mar 29, 2012 at 06:56:30PM -0400, Tom Lane wrote: Marko Kreen mark...@gmail.com writes: Second conclusion is that current dblink row-processor usage is broken when user uses multiple

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-30 Thread Marko Kreen
On Fri, Mar 30, 2012 at 7:04 PM, Marko Kreen mark...@gmail.com wrote: Have you looked at the examples?  PQgetResult() is pretty good hint that one resultset finished... Ok, the demos are around this long thread and hard to find, so here is a summary of links: Original design mail:

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-30 Thread Tom Lane
Marko Kreen mark...@gmail.com writes: On Wed, Mar 07, 2012 at 03:14:57PM +0900, Kyotaro HORIGUCHI wrote: My suggestion - check in getAnotherTuple whether resultStatus is already error and do nothing then. This allows internal pqAddRow to set regular out of memory error. Otherwise give

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-30 Thread Marko Kreen
On Fri, Mar 30, 2012 at 05:18:42PM -0400, Tom Lane wrote: Marko Kreen mark...@gmail.com writes: On Wed, Mar 07, 2012 at 03:14:57PM +0900, Kyotaro HORIGUCHI wrote: My suggestion - check in getAnotherTuple whether resultStatus is already error and do nothing then. This allows internal

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-30 Thread Tom Lane
Marko Kreen mark...@gmail.com writes: On Fri, Mar 30, 2012 at 05:18:42PM -0400, Tom Lane wrote: I'm pretty dissatisfied with the error reporting situation for row processors. You can't just decide not to solve it, which seems to be the current state of affairs. What I'm inclined to do is to

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-30 Thread Marko Kreen
On Sat, Mar 31, 2012 at 1:13 AM, Tom Lane t...@sss.pgh.pa.us wrote: Marko Kreen mark...@gmail.com writes: On Fri, Mar 30, 2012 at 05:18:42PM -0400, Tom Lane wrote: I'm pretty dissatisfied with the error reporting situation for row processors.  You can't just decide not to solve it, which seems

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-30 Thread Tom Lane
Marko Kreen mark...@gmail.com writes: On Sat, Mar 31, 2012 at 1:13 AM, Tom Lane t...@sss.pgh.pa.us wrote: Not if the message is a constant string, which seems like the typical situation (think out of memory).  If the row processor does need a buffer for a constructed string, it could make use

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-29 Thread Tom Lane
Kyotaro HORIGUCHI horiguchi.kyot...@oss.ntt.co.jp writes: I'm sorry to have coded a silly bug. The previous patch has a bug in realloc size calculation. And separation of the 'connname patch' was incomplete in regtest. It is fixed in this patch. I've applied a modified form of the conname

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-29 Thread Tom Lane
Marko Kreen mark...@gmail.com writes: My conclusion is that row-processor API is low-level expert API and quite easy to misuse. It would be preferable to have something more robust as end-user API, the PQgetRow() is my suggestion for that. Thus I see 3 choices: 1) Push row-processor as main

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-29 Thread Kyotaro HORIGUCHI
I've applied a modified form of the conname update patch. It seemed to me that the fault is really in the DBLINK_GET_CONN and DBLINK_GET_NAMED_CONN macros, which ought to be responsible for setting the surrounding function's conname variable along with conn, rconn, etc. There was actually a

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-27 Thread Marko Kreen
On Sat, Mar 24, 2012 at 02:22:24AM +0200, Marko Kreen wrote: Main advantage of including PQgetRow() together with low-level rowproc API is that it allows de-emphasizing more complex parts of rowproc API (exceptions, early exit, skipresult, custom error msg). And drop/undocument them or simply

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-26 Thread Kyotaro HORIGUCHI
Hello, This is new version of patch for dblink using row processor. - Use palloc to allocate temporaly memoriy blocks. - Memory allocation is now done in once. Preallocate a block of initial size and palloc simplified reallocation code. - Resurrected the route for command invoking. And

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-26 Thread Kyotaro HORIGUCHI
I'm sorry to have coded a silly bug. The previous patch has a bug in realloc size calculation. And separation of the 'connname patch' was incomplete in regtest. It is fixed in this patch. regards, -- Kyotaro Horiguchi NTT Open Source Software Center diff --git a/contrib/dblink/dblink.c

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-23 Thread Kyotaro HORIGUCHI
Thank you for picking up. is considering three cases: it got a 2-byte integer (and can continue on), or there aren't yet 2 more bytes available in the buffer, in which case it should return EOF without doing anything, or pqGetInt detected a hard error and updated the connection error state

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-23 Thread Marko Kreen
I saw Kyotaro already answered, but I give my view as well. On Thu, Mar 22, 2012 at 06:07:16PM -0400, Tom Lane wrote: AFAICT it breaks async processing entirely, because many changes have been made that fail to distinguish insufficient data available as yet from hard error. As an example,

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-22 Thread Tom Lane
Marko Kreen mark...@gmail.com writes: [ patches against libpq and dblink ] I think this patch needs a lot of work. AFAICT it breaks async processing entirely, because many changes have been made that fail to distinguish insufficient data available as yet from hard error. As an example, this

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-07 Thread Kyotaro HORIGUCHI
Hello, #- We expect PQisBusy(), PQconsumeInput()(?) and #- PQgetResult() to exit immediately and we can #- call PQgetResult(), PQskipResult() or #- PQisBusy() after. | 1 - OK (I'm done with the row) |

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-06 Thread Marko Kreen
On Tue, Mar 06, 2012 at 11:13:45AM +0900, Kyotaro HORIGUCHI wrote: But it's broken in V3 protocol - getAnotherTuple() will be called only if the packet is fully read. If the packet contents do not agree with packet header, it's protocol error. Only valid EOF return in V3

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-06 Thread Kyotaro HORIGUCHI
Hello, But I don't understand how to secure the rows (or table data) fully loaded at the point of getAnotherTuple called... I found how pqParseInput ensures the entire message is loaded before getAnotherTuple called. fe-protocol3.c:107 | avail = conn-inEnd - conn-inCursor; | if (avail

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-06 Thread Kyotaro HORIGUCHI
Hello, Nevertheless, the problem that exit from parseInput() caused by non-zero return of getAnotherTuple() results in immediate re-enter into getAnotherTuple() via parseInput() and no other side effect is still there. But I will do that in the next patch, though. So now I convinced that the

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-06 Thread Kyotaro HORIGUCHI
Hello, this is new version of the patch. # early_exit.diff is not included for this time and maybe also # later. The set of the return values of PQrowProcessor looks # unnatural if the 0 is removed. * Convert old EOFs to protocol errors in V3 getAnotherTuple() done. * V2 getAnotherTuple()

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-05 Thread Kyotaro HORIGUCHI
Hello, I'm sorry for the abesnce. But it's broken in V3 protocol - getAnotherTuple() will be called only if the packet is fully read. If the packet contents do not agree with packet header, it's protocol error. Only valid EOF return in V3 getAnotherTuple() is when row processor asks for

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-03-01 Thread Marko Kreen
On Tue, Feb 28, 2012 at 05:04:44PM +0900, Kyotaro HORIGUCHI wrote: There is still one EOF in v3 getAnotherTuple() - pqGetInt(tupnfields), please turn that one also to protocolerror. pqGetInt() returns EOF only when it wants additional reading from network if the parameter `bytes' is

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-28 Thread Kyotaro HORIGUCHI
This is the new version of the patch. It is not rebased to the HEAD because of a build error. It's better to restore old two-path error handling. I restorerd OOM and save result route. But it seems needed to get back any amount of memory on REAL OOM as the comment in original code says. So I

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-27 Thread Kyotaro HORIGUCHI
Hello, On OOM, the old result is freed to have higher chance that constructing new result succeeds. But if we want to transport error message, we need to keep old PGresult around. Thus two separate paths. Ok, I understood the aim. But now we can use non-local exit to do that for not only

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-27 Thread Marko Kreen
On Mon, Feb 27, 2012 at 05:20:30PM +0900, Kyotaro HORIGUCHI wrote: Hello, On OOM, the old result is freed to have higher chance that constructing new result succeeds. But if we want to transport error message, we need to keep old PGresult around. Thus two separate paths. Ok, I

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-27 Thread Kyotaro HORIGUCHI
Hello. I will show you fixed version patch later, please wait for a while. == It's more important to not destabilize V3 code. Ok, I withdraw that agreeing with that point. And I noticed that the proposal before is totally a crap becuase I have mixed up asyncStatus with resultStatus in it.

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-26 Thread Marko Kreen
On Fri, Feb 24, 2012 at 05:46:16PM +0200, Marko Kreen wrote: - rename to PQrecvRow() and additionally provide PQgetRow() I tried it and it seems to work as API - there is valid behaviour for both sync and async connections. Sync connection - PQgetRow() waits for data from network: if

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-24 Thread Kyotaro HORIGUCHI
Hello, this is new version of the patch. By the way, I would like to ask you one question. What is the reason why void* should be head or tail of the parameter list? Aesthetical reasons: I got it. Thank you. Last comment - if we drop the plan to make PQsetRowProcessorErrMsg() usable

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-24 Thread Marko Kreen
On Fri, Feb 24, 2012 at 07:53:14PM +0900, Kyotaro HORIGUCHI wrote: Hello, this is new version of the patch. By the way, I would like to ask you one question. What is the reason why void* should be head or tail of the parameter list? Aesthetical reasons: I got it. Thank you.

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-24 Thread Marko Kreen
On Tue, Feb 14, 2012 at 01:39:06AM +0200, Marko Kreen wrote: I tried imaging some sort of getFoo() style API for fetching in-flight row data, but I always ended up with rewrite libpq step, so I feel it's not productive to go there. Instead I added simple feature: rowProcessor can return '2',

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-23 Thread Kyotaro HORIGUCHI
Hello, this is new version of the patch. # This patch is based on the commit # 2bbd88f8f841b01efb073972b60d4dc1ff1f6fd0 @ Feb 13 to avoid the # compile error caused by undeclared LEAKPROOF in kwlist.h. It must free the PGresults that PQgetResult() returns. I'm sorry. It slipped out of my

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-23 Thread Marko Kreen
On Thu, Feb 23, 2012 at 07:14:03PM +0900, Kyotaro HORIGUCHI wrote: Hello, this is new version of the patch. Looks good. By the way, I would like to ask you one question. What is the reason why void* should be head or tail of the parameter list? Aesthetical reasons: 1) PGresult and

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-21 Thread Kyotaro HORIGUCHI
Hello, I don't have any attachment to PQskipRemainingResults(), but I think that some formal method to skip until Command-Complete('C') without breaking session is necessary because current libpq does so. We have such function: PQgetResult(). Only question is how to flag that the

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-21 Thread Kyotaro HORIGUCHI
Sorry, I should fix a wrong word selection.. Let me confirm the effects of this function. Is the below description right? - PQskipResult(conn, false) makes just following PQgetResult() to skip current bunch of rows and the consequent PQgetResult()'s gathers rows as usual. -

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-21 Thread Marko Kreen
On Tue, Feb 21, 2012 at 11:42 AM, Kyotaro HORIGUCHI horiguchi.kyot...@oss.ntt.co.jp wrote: Also, as row handler is on connection, then changing it's behavior is connection context, not result. OK, current implement copying the pointer to the row processor from PGconn to PGresult and

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-21 Thread Kyotaro HORIGUCHI
Hello, Note I dropped the row processor from under PGresult. Please don't put it back there. I overlooked that. I understand it. This seems also can be done by the return value of PQsetRowProcessor() (currently void). Anyway, I provide this function and also change the return value of

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-21 Thread Marko Kreen
On Tue, Feb 21, 2012 at 12:13 PM, Kyotaro HORIGUCHI horiguchi.kyot...@oss.ntt.co.jp wrote: - PQskipResult(conn, true) makes all consequent PQgetResult()'s  to skip all the rows. Well, Is this right? Yes, call getResult() until it returns NULL. If this is right, row processor should stay

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-21 Thread Kyotaro HORIGUCHI
Thank you. Everything seems clear. Please wait for a while. PQskipResult: - store old callback and param in local vars - set do-nothing row callback - call PQgetresu On Tue, Feb 21, 2012 at 12:13 PM, Kyotaro HORIGUCHI horiguchi.kyot...@oss.ntt.co.jp wrote: - PQskipResult(conn, true)

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-21 Thread Marko Kreen
On Wed, Feb 22, 2012 at 12:27:57AM +0900, Kyotaro HORIGUCHI wrote: fe-exec.c - PQskipResult() is added instead of PGskipRemainigResults(). It must free the PGresults that PQgetResult() returns. Also, please fix 2 issues mentined here:

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-20 Thread Kyotaro HORIGUCHI
Hello, I don't have any attachment to PQskipRemainingResults(), but I think that some formal method to skip until Command-Complete('C') without breaking session is necessary because current libpq does so. On Thu, Feb 16, 2012 at 02:24:19PM +0900, Kyotaro HORIGUCHI wrote: The choices of

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-20 Thread Marko Kreen
On Tue, Feb 21, 2012 at 02:11:35PM +0900, Kyotaro HORIGUCHI wrote: I don't have any attachment to PQskipRemainingResults(), but I think that some formal method to skip until Command-Complete('C') without breaking session is necessary because current libpq does so. We have such function:

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-18 Thread Marko Kreen
Demos/tests of the new API: https://github.com/markokr/libpq-rowproc-demos Comments resulting from that: - PQsetRowProcessorErrMsg() should take const char* - callback API should be (void *, PGresult *, PQrowValue*) or void* at the end, but not in the middle I have not looked yet what

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-16 Thread Marko Kreen
On Thu, Feb 16, 2012 at 02:24:19PM +0900, Kyotaro HORIGUCHI wrote: As far as I see, on an out-of-memory in getAnotherTuple() makes conn-result-resultStatus = PGRES_FATAL_ERROR and qpParseInputp[23]() skips succeeding 'D' messages consequently. When exception raised within row processor,

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-16 Thread Marko Kreen
On Thu, Feb 16, 2012 at 05:49:34PM +0900, Kyotaro HORIGUCHI wrote: I added the function PQskipRemainingResult() and use it in dblink. This reduces the number of executing try-catch block from the number of rows to one per query in dblink. This implementation is wrong - you must not simply

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-15 Thread Kyotaro HORIGUCHI
Hello, sorry for long absense. As far as I see, on an out-of-memory in getAnotherTuple() makes conn-result-resultStatus = PGRES_FATAL_ERROR and qpParseInputp[23]() skips succeeding 'D' messages consequently. When exception raised within row processor, pg_conn-inCursor always positioned in

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-13 Thread Marko Kreen
On Tue, Feb 07, 2012 at 04:44:09PM +0200, Marko Kreen wrote: Although it seems we could allow exceptions, at least when we are speaking of Postgres backend, as the connection and result are internally consistent state when the handler is called, and the partial PGresult is stored under

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-08 Thread Marko Kreen
On Wed, Feb 08, 2012 at 02:44:13PM +0900, Shigeru Hanada wrote: (2012/02/07 23:44), Marko Kreen wrote: On Tue, Feb 07, 2012 at 07:25:14PM +0900, Shigeru Hanada wrote: - What is the right (or recommended) way to prevent from throwing exceptoin in row-processor callback function? When author

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-07 Thread Shigeru Hanada
(2012/02/02 23:30), Marko Kreen wrote: On Thu, Feb 02, 2012 at 04:51:37PM +0900, Kyotaro HORIGUCHI wrote: Hello, This is new version of dblink.c - Memory is properly freed when realloc returns NULL in storeHandler(). - The bug that free() in finishStoreInfo() will be fed with garbage

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-07 Thread Marko Kreen
On Tue, Feb 07, 2012 at 07:25:14PM +0900, Shigeru Hanada wrote: (2012/02/02 23:30), Marko Kreen wrote: On Thu, Feb 02, 2012 at 04:51:37PM +0900, Kyotaro HORIGUCHI wrote: Hello, This is new version of dblink.c - Memory is properly freed when realloc returns NULL in storeHandler(). -

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-07 Thread Robert Haas
On Tue, Feb 7, 2012 at 9:44 AM, Marko Kreen mark...@gmail.com wrote: - What is the right (or recommended) way to prevent from throwing exceptoin in row-processor callback function?  When author should use PG_TRY block to catch exception in the callback function? When it calls backend

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-07 Thread Shigeru Hanada
(2012/02/07 23:44), Marko Kreen wrote: On Tue, Feb 07, 2012 at 07:25:14PM +0900, Shigeru Hanada wrote: - What is the right (or recommended) way to prevent from throwing exceptoin in row-processor callback function? When author should use PG_TRY block to catch exception in the callback

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-02 Thread Marko Kreen
On Thu, Feb 02, 2012 at 04:51:37PM +0900, Kyotaro HORIGUCHI wrote: Hello, This is new version of dblink.c - Memory is properly freed when realloc returns NULL in storeHandler(). - The bug that free() in finishStoreInfo() will be fed with garbage pointer when malloc for sinfo-valbuflen

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-02 Thread Kyotaro HORIGUCHI
Hello, Thanks, merged. I also did some minor coding style cleanups. Thank you for editing many comments and some code I'd left unchanged from my carelessness, and lack of understanding your comments. I'll be more careful about that... There is some potential for experimenting with more

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-01 Thread Kyotaro HORIGUCHI
Hello, Another thing: if realloc() fails, the old pointer stays valid. So it needs to be assigned to temp variable first, before overwriting old pointer. mmm. I've misunderstood of the realloc.. I'll fix there in the next patch. And seems malloc() is preferable to palloc() to avoid

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-01 Thread Marko Kreen
On Wed, Feb 1, 2012 at 10:32 AM, Kyotaro HORIGUCHI horiguchi.kyot...@oss.ntt.co.jp wrote: Another thing: if realloc() fails, the old pointer stays valid. So it needs to be assigned to temp variable first, before overwriting old pointer.  mmm. I've misunderstood of the realloc.. I'll fix there

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-02-01 Thread Kyotaro HORIGUCHI
Hello, This is new version of dblink.c - Memory is properly freed when realloc returns NULL in storeHandler(). - The bug that free() in finishStoreInfo() will be fed with garbage pointer when malloc for sinfo-valbuflen fails is fixed. regards, -- Kyotaro Horiguchi NTT Open Source Software

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-01-30 Thread Kyotaro HORIGUCHI
Thank you for comments, this is revised version of the patch. The gain of performance is more than expected. Measure script now does query via dblink ten times for stability of measuring, so the figures become about ten times longer than the previous ones. sec% to

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-01-30 Thread horiguchi . kyotaro
I'm sorry. Thank you for comments, this is revised version of the patch. The malloc error handling in dblink.c of the patch is broken. It is leaking memory and breaking control. i'll re-send the properly fixed patch for dblink.c later. # This severe back pain should have made me stupid :-p

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-01-30 Thread Marko Kreen
On Mon, Jan 30, 2012 at 06:06:57PM +0900, Kyotaro HORIGUCHI wrote: The gain of performance is more than expected. Measure script now does query via dblink ten times for stability of measuring, so the figures become about ten times longer than the previous ones. sec

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-01-30 Thread Kyotaro HORIGUCHI
This is fixed version of dblink.c for row processor. i'll re-send the properly fixed patch for dblink.c later. - malloc error in initStoreInfo throws ERRCODE_OUT_OF_MEMORY. (new error) - storeHandler() now returns FALSE on malloc failure. Garbage cleanup is done in dblink_fetch() or

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-01-30 Thread Marko Kreen
On Tue, Jan 31, 2012 at 4:59 AM, Kyotaro HORIGUCHI horiguchi.kyot...@oss.ntt.co.jp wrote: This is fixed version of dblink.c for row processor. i'll re-send the properly fixed patch for dblink.c later. - malloc error in initStoreInfo throws ERRCODE_OUT_OF_MEMORY. (new error) - storeHandler()

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-01-27 Thread Kyotaro HORIGUCHI
Hello, This is a new version of the patch formerly known as 'alternative storage for libpq'. - Changed the concept to 'Alternative Row Processor' from 'Storage handler'. Symbol names are also changed. - Callback function is modified following to the comment. - From the restriction of time, I

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-01-27 Thread Merlin Moncure
On Fri, Jan 27, 2012 at 2:57 AM, Kyotaro HORIGUCHI horiguchi.kyot...@oss.ntt.co.jp wrote: Hello, This is a new version of the patch formerly known as 'alternative storage for libpq'. I took a quick look at the patch and the docs. Looks good and agree with rationale and implementation. I see

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-01-27 Thread Marko Kreen
On Fri, Jan 27, 2012 at 05:57:01PM +0900, Kyotaro HORIGUCHI wrote: Hello, This is a new version of the patch formerly known as 'alternative storage for libpq'. - Changed the concept to 'Alternative Row Processor' from 'Storage handler'. Symbol names are also changed. - Callback function

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-01-27 Thread Marko Kreen
On Fri, Jan 27, 2012 at 09:35:04AM -0600, Merlin Moncure wrote: On Fri, Jan 27, 2012 at 2:57 AM, Kyotaro HORIGUCHI - The meaning of PGresAttValue is changed. The field 'value' now  contains a value withOUT terminating zero. This change seems to  have no effect on any other portion within

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-01-26 Thread Kyotaro HORIGUCHI
Thank you for the comment, First, my priority is one-the-fly result processing, not the allocation optimizing. And this patch seems to make it possible, I can process results row-by-row, without the need to buffer all of them in PQresult. Which is great! But the current API seems clumsy,

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-01-23 Thread Marko Kreen
On Sat, Jan 21, 2012 at 1:52 PM, Marc Mamin m.ma...@intershop.de wrote: c. Refine error handling of dblink.c. I think it preserves the    previous behavior for column number mismatch and type    conversion exception. Hello, I don't know if this cover following issue. I just mention it

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-01-21 Thread Marc Mamin
c. Refine error handling of dblink.c. I think it preserves the previous behavior for column number mismatch and type conversion exception. Hello, I don't know if this cover following issue. I just mention it for the case you didn't notice it and would like to handle this rather

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-01-20 Thread Marko Kreen
On Tue, Jan 17, 2012 at 05:53:33PM +0900, Kyotaro HORIGUCHI wrote: Hello, This is revised and rebased version of the patch. a. Old term `Add Tuple Function' is changed to 'Store Handler'. The reason why not `storage' is simply length of the symbols. b. I couldn't find the place to

Re: [HACKERS] Speed dblink using alternate libpq tuple storage

2012-01-17 Thread Kyotaro HORIGUCHI
Hello, This is revised and rebased version of the patch. a. Old term `Add Tuple Function' is changed to 'Store Handler'. The reason why not `storage' is simply length of the symbols. b. I couldn't find the place to settle PGgetAsCString() in. It is removed and storeHandler()@dblink.c

[HACKERS] Speed dblink using alternate libpq tuple storage

2012-01-13 Thread Greg Smith
One patch that fell off the truck during a turn in the November CommitFest was Allow substitute allocators for PGresult. Re-reading the whole thing again, it actually turned into a rather different submission in the middle, and I know I didn't follow that shift correctly then. I'm replying