Re: Speed dblink using alternate libpq tuple storage

From: Kyotaro HORIGUCHI <horiguchi(dot)kyotaro(at)oss(dot)ntt(dot)co(dot)jp>
To: markokr(at)gmail(dot)com
Cc: greg(at)2ndquadrant(dot)com, pgsql-hackers(at)postgresql(dot)org, mmoncure(at)gmail(dot)com, shigeru(dot)hanada(at)gmail(dot)com
Subject: Re: Speed dblink using alternate libpq tuple storage
Date: 2012-03-07 06:14:57
Message-ID: 20120307.151457.91509043.horiguchi.kyotaro@oss.ntt.co.jp
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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() can leak PGresult when handling custom
> error from row processor.

Custom error message is removed from both V2 and V3.

> * 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()..

> * 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().

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

done.

> 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?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center

Attachment Content-Type Size
libpq_rowproc_20120307.patch text/x-patch 22.1 KB
libpq_rowproc_doc_20120307.patch text/x-patch 8.1 KB
dblink_use_rowproc_20120307.patch text/x-patch 13.5 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Fujii Masao 2012-03-07 07:07:37 Re: Scaling XLog insertion (was Re: Moving more work outside WALInsertLock)
Previous Message Pavel Stehule 2012-03-07 05:35:06 Re: review: CHECK FUNCTION statement