Re: Review: ECPG infrastructure changes part 1, was: Re: ECPG fixes - Mailing list 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/