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).
> - 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.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
* 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...
In response to
pgsql-hackers by date
|Next:||From: Robert Haas||Date: 2012-01-30 18:19:40|
|Subject: Re: [v9.2] Add GUC sepgsql.client_label|
|Previous:||From: Josh Berkus||Date: 2012-01-30 18:06:09|
|Subject: Re: Hot standby off of hot standby?|