Re: Speed dblink using alternate libpq tuple storage

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 % to Original
> Original : 31.5 100.0%
> RowProcessor patch : 31.3 99.4%
> dblink patch : 24.6 78.1%
>
> RowProcessor patch alone makes no loss or very-little gain, and
> full patch gives us 22% gain for the benchmark(*1).

Excellent!

> - The callers of RowProcessor() no more set null_field to
> PGrowValue.value. Plus, the PGrowValue[] which RowProcessor()
> receives has nfields + 1 elements to be able to make rough
> estimate by cols->value[nfields].value - cols->value[0].value -
> something. The somthing here is 4 * nfields for protocol3 and
> 4 * (non-null fields) for protocol2. I fear that this applies
> only for textual transfer usage...

Excact estimate is not important here. And (nfields + 1) elem
feels bit too much magic, considering that most users probably
do not need it. Without it, the logic would be:

total = last.value - first.value + ((last.len > 0) ? last.len : 0)

which isn't too complex. So I think we can remove it.

= Problems =

* Remove the dubious memcpy() in pqAddRow()

* I think the dynamic arrays in getAnotherTuple() are not portable enough,
please do proper allocation for array. I guess in PQsetResultAttrs()?

= Minor notes =

These can be argued either way, if you don't like some
suggestion, you can drop it.

* Move PQregisterRowProcessor() into fe-exec.c, then we can make
pqAddRow static.

* Should PQclear() free RowProcessor error msg? It seems
it should not get outside from getAnotherTuple(), but
thats not certain. Perhaps it would be clearer to free
it here too.

* Remove the part of comment in getAnotherTuple():
* Buffer content may be shifted on reloading additional
* data. So we must set all pointers on every scan.

It's confusing why it needs to clarify that, as there
is nobody expecting it.

* PGrowValue documentation should mention that ->value pointer
is always valid.

* dblink: Perhaps some of those mallocs() could be replaced
with pallocs() or even StringInfo, which already does
the realloc dance? I'm not familiar with dblink, and
various struct lifetimes there so I don't know it that
actually makes sense or not.

It seems this patch is getting ReadyForCommitter soon...

--
marko