Review: ECPG infrastructure changes part 1, was: Re: ECPG fixes - Mailing list pgsql-hackers

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.

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
stringwill be truncated at
 
149 characters expanded.  */
/home/anton/cybertec/patches/ecpq/23.patch:4019: trailing whitespace.       exec sql close :curname;



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.


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.

// 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
> 
> 
> 
>