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

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 appropreate. Non-zero return
> from it seems should be handled as EOF, not a protocol error. The
> one point I had modified bugilly is also restored. The so-called
> 'protocol error' has been vanished eventually.

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 early exit.

> Is there someting left undone?

* Convert old EOFs to protocol errors in V3 getAnotherTuple()

* V2 getAnotherTuple() can leak PGresult when handling custom error from row processor.

* remove pqIsnonblocking(conn) check when row processor returned 2. I missed that it's valid to call
PQisBusy/PQconsumeInput/PQgetResulton sync connection.
 

* 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")

* Please drop PQsetRowProcessorErrMsg() / PQresultSetErrMsg(). Main problem is that it needs to be synced with error
handlingin rest of libpq, which is unlike the rest of row processor patch, which consists only of local changes.  All
solutionshere are either ugly hacks or too complex to be part of this patch.
 

Also considering that we have working exceptions and PQgetRow,
I don't see much need for custom error messages.  If really needed,
it should be introduced as separate patch, as the area of code it
affects is completely different.

Currently the custom error messaging seems to be the blocker for
this patch, because of raised complexity when implementing it and
when reviewing it.  Considering how unimportant the provided
functionality is, compared to rest of the patch, I think we should
simply drop it.

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".

> By the way, I noticed that dblink always says that the current
> connection is 'unnamed' in messages the errors in
> dblink_record_internal@dblink.  I could see that
> dblink_record_internal defines the local variable conname = NULL
> and pass it to dblink_res_error to display the error message. But
> no assignment on it in the function.
> 
> It seemed properly shown when I added the code to set conname
> from PG_GETARG_TEXT_PP(0) if available, in other words do that
> just after DBLINK_GET_CONN/DBLINK_GET_NAMED_CONN's. It seems the
> dblink's manner...  This is not included in this patch.
> 
> Furthurmore dblink_res_error looks only into returned PGresult to
> display the error and always says only `Error occurred on dblink
> connection..: could not execute query'..
> 
> Is it right to consider this as follows?
> 
>  - dblink is wrong in error handling. A client of libpq should
>    see PGconn by PQerrorMessage() if (or regardless of whether?)
>    PGresult says nothing about error.

Yes, it seems like bug.

-- 
marko