Thanks for the review.
2013-12-03 16:48 keltezéssel, Antonin Houska írta:
The changes are very well divided into logical units, so I think I could
understand the ideas. I'm not too familiar with the ecpg internals, so
consider this at least a coding review.
git apply: Clean, except for one finding below
Build: no errors/warnings
Documentation build: no errors/warnings. The changes do appear in ecpg.html.
Regression tests: all passed
Non-Linux platforms: I can't verify, but I haven't noticed any new
dependencies added.
Comments (in the source code): good.
(My) additional comments:
22.patch
--------
tuples_left > 0
instead of just
tuples_left
seems to me safer in for-loops. Not sure if there's a convention for
this though.
I'll look at it, maybe the >0 had a reason.
23.patch
--------
git apply --verbose ~/cybertec/patches/ecpq/23.patch
/home/anton/cybertec/patches/ecpq/23.patch:494: space before tab in indent.
/*------
/home/anton/cybertec/patches/ecpq/23.patch:495: space before tab in indent.
translator: this string will be truncated at
149 characters expanded. */
/home/anton/cybertec/patches/ecpq/23.patch:4019: trailing whitespace.
exec sql close :curname;
I will fix the extra spaces.
Tests - 23.patch
----------------
src/interfaces/ecpg/test/sql/cursorsubxact.pgc
/*
* Test the implicit RELEASE SAVEPOINT if a SAVEPOINT
* is used with an already existing name.
*/
Shouldn't it be "... if a CURSOR is used with an already existing
name?". Or just "... implicit RELEASE SAVEPOINT after an error"?
I'd also appreciate a comment where exactly the savepoint is
(implicitly) released.
If you do this:
SAVEPOINT A;
<some operations>
SAVEPOINT A; /* same savepoint name */
then the operations between the two are implicitly committed
to the higher subtransaction, i.e. it works as if there was a
RELEASE SAVEPOINT A; before the second SAVEPOINT A;
It happens to be tested with a DECLARE CURSOR statement
and the runtime has to refresh its knowledge about the cursor
by propagating it into a subtransaction one level higher.
23.patch and 24.patch
---------------------
SO_MAJOR_VERSION and also interfaces/ecpg/include/ecpglib.h is changed
Thus all client applications probably need to be preprocessed & compiled
against the PG 9.4. I don't know how this is usually enforced. I'm
mentioning it for the sake of completeness.
The soversion has changed because of the changes in already
existing exported functions.
// Antonin Houska (Tony)
On 11/28/2013 03:21 PM, Boszormenyi Zoltan wrote:
2013-11-20 14:53 keltezéssel, Boszormenyi Zoltan írta:
2013-11-20 14:41 keltezéssel, Boszormenyi Zoltan írta:
2013-11-12 07:15 keltezéssel, Boszormenyi Zoltan írta:
2013-11-12 07:01 keltezéssel, Noah Misch írta:
On Mon, Nov 11, 2013 at 10:17:54AM +0100, Boszormenyi Zoltan wrote:
The old contents of my GIT repository was removed so you need to
clone it fresh. https://github.com/zboszor/ecpg-readahead.git
I won't post the humongous patch again, since sending a 90KB
compressed file to everyone on the list is rude.
Patches of that weight show up on a regular basis. I don't think it's rude.
OK, here it is.
...
Subsequent patches will come as reply to this email.
Infrastructure changes in ecpglib/execute.c to split up
ECPGdo and ecpg_execute() and expose the parts as
functions internal to ecpglib.
Rebased after killing the patch that changed the DECLARE CURSOR command tag.
All infrastructure patches are attached, some of them compressed.
Best regards,
Zoltán Böszörményi
--
----------------------------------
Zoltán Böszörményi
Cybertec Schönig & Schönig GmbH
Gröhrmühlgasse 26
A-2700 Wiener Neustadt, Austria
Web: http://www.postgresql-support.de
http://www.postgresql.at/
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
