Re: Speed dblink using alternate libpq tuple storage

On Wed, Mar 07, 2012 at 03:14:57PM +0900, Kyotaro HORIGUCHI wrote:
> > * remove pqIsnonblocking(conn) check when row processor returned 2.
> > I missed that it's valid to call PQisBusy/PQconsumeInput/PQgetResult
> > on sync connection.
>
> done. This affects PQisBusy, but PQgetResult won't be affected as
> far as I see. I have no idea for PQconsumeInput()..

PQconsumeInput does not parse the row and there is no need to
do anything with PQgetResult().

> > * It seems the return codes from callback should be remapped,
> > (0, 1, 2) is unusual pattern. Better would be:
> >
> > -1 - error
> > 0 - stop parsing / early exit ("I'm not done yet")
> > 1 - OK ("I'm done with the row")
>
> done.
>
> This might be described more precisely as follows,
>
> | -1 - error - erase result and change result status to
> | - FATAL_ERROR All the rest rows in current result
> | - will skipped(consumed).
> | 0 - stop parsing / early exit ("I'm not done yet")
> | - getAnotherTuple returns EOF without dropping PGresult.
> # - 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")
> | - save result and getAnotherTuple returns 0.
>
> The lines prefixed with '#' is the desirable behavior I have
> understood from the discussion so far. And I doubt that it works
> as we expected for PQgetResult().

No, the desirable behavior is already implemented and documented -
the "stop parsing" return code affects only PQisBusy(). As that
is the function that does the actual parsing.

The main plus if such scheme is that we do not change the behaviour
of any existing APIs.

> > * Please drop PQsetRowProcessorErrMsg() / PQresultSetErrMsg().
>
> done.

You optimized libpq_gettext() calls, but it's wrong - they must
wrap the actual strings so that the strings can be extracted
for translating.

Fixed in attached patch.

> > 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 generic
> > "row processor error".
>
> Current implement seems already doing this in
> parseInput3(). Could you give me further explanation?

The suggestion was about getAnotherTuple() - currently it sets
always "error in row processor". With such check, the callback
can set the error result itself. Currently only callbacks that
live inside libpq can set errors, but if we happen to expose
error-setting function in outside API, then the getAnotherTuple()
would already be ready for it.

See attached patch, I did some generic comment/docs cleanups
but also minor code cleanups:

- PQsetRowProcessor(NULL,NULL) sets Param to PGconn, instead NULL,
this makes PGconn available to pqAddRow.

- pqAddRow sets "out of memory" error itself on PGconn.

- getAnotherTuple(): when callback returns -1, it checks if error
message is set, does nothing then.

- put libpq_gettext() back around strings.

- dropped the error_saveresult label, it was unnecessary branch.

- made functions survive conn==NULL.

- dblink: changed skipAll parameter for PQskipResult() to TRUE,
as dblink uses PQexec which can send several queries.

- dblink: refreshed regtest result, as now we get actual
connection name in error message.

- Synced PQgetRow patch with return value changes.

- Synced demos at https://github.com/markokr/libpq-rowproc-demos
with return value changes.

I'm pretty happy with current state. So tagging it
ReadyForCommitter.

--
marko