Re: Speed dblink using alternate libpq tuple storage - Mailing list pgsql-hackers

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 understood the aim. But now we can use non-local exit to do
> that for not only asynchronous reading (PQgetResult()) but
> synchronous (PQexec()). If we should provide a means other than
> exceptions to do that, I think it should be usable for both
> syncronous and asynchronous reading. conn->asyncStatus seems to
> be used for the case.
> 
> Wow is the modification below?
> 
> - getAnotherTuple() now returns 0 to continue as before, and 1
>   instead of EOF to signal EOF state, and 2 to instruct to exit
>   immediately.
> 
> - pqParseInput3 set conn->asyncStatus to PGASYNC_BREAK for the
>   last case,
> 
> - then PQgetResult() returns immediately when
>   asyncStatus == PGASYNC_TUPLES_BREAK after parseInput() retunes.
> 
> - and PQexecFinish() returns immediately if PQgetResult() returns
>   with aysncStatys == PGASYNC_TUPLES_BREAK.
> 
> - PGgetResult() sets asyncStatus = PGRES_TUPLES_OK if called with
>   asyncStatus == PGRES_TUPLES_BREAK
> 
> - New libpq API PQisBreaked(PGconn*) returns if asyncStatus ==
>   PGRES_TUPLES_BREAK can be used to check if the transfer is breaked.

I don't get where are you going with such changes.  Are you trying to
make V2 code survive row-processor errors?  (Only V2 has concept of
"EOF state".)  Then forget about it.  It's more important to not
destabilize V3 code.

And error from row processor is not something special from other errors.
Why does it need special state?  And note that adding new PGASYNC or
PGRES state needs review of all if() and switch() statements in the
code that use those fields...  So there must serious need for it.
At the moment I don't see such need.

I just asked you to replace ->rowProcessorErrMsg with ->errMsg
to get rid of unnecessary field.

But if you want to do bigger cleanup, then you can instead make
PQsetRowProcessorErrMsg() more generic:
 PQsetErrorMessage(PGconn *conn, const char *msg)

Current code has the tiny problem that it adds new special state between
PQsetRowProcessorErrMsg() and return from callback to getAnotherTuple()
where getAnotherTuple() sets actual error state.  When the callback
exits via exception, the state can leak to other code.  It should not
break anything, but it's still annoying.

Also, with the PQgetRow() patch, the need for doing complex processing
under callback has decreased and the need to set error outside callback
has increased.

As a bonus, such generic error-setting function would lose any extra
special state introduced by row-processor patch.

Previously I mentioned that callback would need to have additional
PGconn* argument to make connection available to callback to use such
generic error setting function, but now I think it does not need it -
simple callbacks don't need to set errors and complex callback can make
the PGconn available via Param.  Eg. the internal callback should set
Param to PGconn, instead keeping NULL there.

-- 
marko