From: | Boszormenyi Zoltan <zb(at)cybertec(dot)at> |
---|---|
To: | Antonin Houska <antonin(dot)houska(at)gmail(dot)com>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Michael Meskes <meskes(at)postgresql(dot)org> |
Cc: | Noah Misch <noah(at)leadboat(dot)com>, Peter Eisentraut <peter_e(at)gmx(dot)net>, Robert Haas <robertmhaas(at)gmail(dot)com>, PG Hackers <pgsql-hackers(at)postgresql(dot)org>, Heikki Linnakangas <hlinnakangas(at)vmware(dot)com>, Bruce Momjian <bruce(at)momjian(dot)us>, Hans-Jürgen Schönig <hs(at)cybertec(dot)at> |
Subject: | Re: Review: ECPG infrastructure changes part 1, was: Re: ECPG fixes |
Date: | 2013-12-03 16:53:34 |
Message-ID: | 529E0C8E.4070700@cybertec.at |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
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/
From | Date | Subject | |
---|---|---|---|
Next Message | Heikki Linnakangas | 2013-12-03 16:54:06 | Re: Dynamic configuration via LDAP in postmaster |
Previous Message | Bruce Momjian | 2013-12-03 16:45:55 | Re: pgsql: libpq: change PQconndefaults() to ignore invalid service files |