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
guessin 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
notcertain. 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
wemust 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
reallocdance? I'm not familiar with dblink, and various struct lifetimes there so I don't know it that actually makes
senseor not.
It seems this patch is getting ReadyForCommitter soon...
--
marko
