Review: ECPG FETCH readahead

I haven't been too familiar with the ECPG internals so far but tried to
do my best.

Generic criteria
----------------

* Does it follow the project coding guidelines?

Yes.

* Are there portability issues?

Shouldn't be. I even noticed the code tries to avoid platform-specific
behaviour of standard library function - see comment about strtoll() in
Linux in 25.patch. (I personally don't know how that function works
elsewhere but that shouldn't matter.)

* Are the comments sufficient and accurate?

Yes, mostly. Just a few improvements recommended below.

* Does it do what it says, correctly?
IMO, yes.

* Does it produce compiler warnings?
No.

* Can you make it crash?
No.

Only some of the parts deserve comments:

23.patch
--------

Reviewed earlier as a component of the relate patch
(http://www.postgresql.org/message-id/52A1E61A.7010100@gmail.com)
and minimum changes done since that time. Nevertheless, a few more comments:

* How about a regression test for the new ECPGcursor_dml() function?

* ECPGtrans() - arguments are explained, but the return (bool) value
should be as well.

* line breaks (pgindent might help):

static void
output_cursor_name(struct cursor *ptr)
{

instead of

static void output_cursor_name(struct cursor *ptr)
{

* confused messages in src/interfaces/ecpg/test/sql/cursorsubxact.pgc,
starting at 100:

exec sql open :curname;
if (sqlca.sqlcode < 0)
printf("open %s (case sensitive) failed with SQLSTATE
%5s\n", curname, sqlca.sqlstate);
else
printf("close %s (case sensitive) succeeded\n", curname);

I suppose both should be "open".

26.patch (the READAHEAD feature itself)
---------------------------------------

I tried to understand the code but couldn't find any obvious error. The
coding style looks clean. Maybe just the arguments and return value of
the ecpglib functinons (ECPGopen, ECPGopen, ECPGfetch, ...) deserve a
bit more attention.

As for tests, I find them comprehensive and almost everything they do is
clear to me. Just the following was worth questions:

* sql-cursor-ra-fetch.stderr

[NO_PID]: ecpg_execute on line 169: query: move forward all in
scroll_cur; with 0 parameter(s) on connection regress1
...
[NO_PID]: ecpg_execute on line 169: query: move relative -3 in
scroll_cur; with 0 parameter(s) on

As the first iteration is special anyway, wouldn't "move absolute -3" be
more efficient than the existing 2 commands?

The following test (also FETCH RELATIVE) uses "move absolute":

[NO_PID]: ecpg_execute on line 186: query: move absolute -20 in
scroll_cur; with 0 parameter(s) on connection regress1

Other than this, I had an idea to improve the behaviour if READAHEAD is
smaller than the actual step, but then realized that 29.patch actually
does fix that :-)

* cursor-ra-move.pgc

What's the relevant difference between unspec_cur1 and unspec_cur2
cursors? There's no difference in scrollability or ordering. And the
tests seem to be identical.

* cursor-ra-swdir.pgc

No questions

* cursorsubxact.pgc

This appears to be the test we discussed earlier:
http://www.postgresql.org/message-id/52A1CAB6.9020600@cybertec.at

The only difference I see is minor change of log message of DECLARE
command. Therefore I didn't have to recheck the logic of the test.

28.patch
--------

* ecpg_cursor_do_move_absolute() and ecpg_cursor_do_move_all() should
better be declared in 26.patch. Just a pedantic comment - all the parts
will probably be applied all at once.

Other
-----

Besides the individual parts I propose some typo fixes and
improvements in wording:

* doc/src/sgml/ecpg.sgml

462c462
< ECPG does cursor accounting in its runtime library and this makes
possible
---
> ECPG does cursor accounting in its runtime library and this makes
it possible
504c504
< recommended to recompile using option <option>-r
readahead=number</option>
---
> recommended to recompile it using option <option>-r
readahead=number</option>

* src/interfaces/ecpg/ecpglib/extern.h

97c97
< * The cursor was created in this level of * (sub-)transaction.
---
> * The cursor was created at this level of (sub-)transaction.

* src/interfaces/ecpg/ecpglib/README.cursor+subxact

4c4
< Contents of tuples returned by a cursor always reflect the data present at
---
> Contents of tuples returned by a cursor always reflects the data
present at
29c29
< needs two operations. If the next command issued by the application spills
---
> need two operations. If the next command issued by the application spills
32c32
< kinds of commands as is after 3 cache misses. FETCH FORWARD/BACKWARD
allows
---
> kinds of commands "as is" after 3 cache misses. FETCH FORWARD/BACKWARD
allows
81c81
< I can also be negative (but also 1-based) if the application started with
---
> It can also be negative (but also 1-based) if the application started with
132c132
< is that (sub-)transactions are also needed to be tracked. These two are
---
> is that (sub-)transactions also need to be tracked. These two are
148c148
< and he issued command may be executed without an error, causing unwanted
---
> and the issued command may be executed without an error, causing unwanted

In addition, please consider the following stuff in
src/interfaces/ecpg/ecpglib/README.cursor+subxact - it can't be
expressed as diff because I'm not 100% sure about the intended meaning

"either implicitly propagated" - I'd expect "either ... or ...". Or
remove no "either" at all?

"Committing a transaction or releasing a subtransaction make cursors ..."
->
"Committing a transaction or releasing a subtransaction propagates the
cursor(s) ... "
?

"The idea behind cursor readahead is to move one tuple less than asked
by the application ..."

My understanding of sql-cursor-ra-fetch.stderr is that the application
does not have to do MOVE explicitly. Maybe

"... to move to the adjacent lower / upper tuple of the supposed
beginning of the result set and then have ecpglib perform FETCH FORWARD
/ BACKWARD N respectively ..."

Consider it just a tentative proposal, I can easily be wrong :-)

That's it for my review.

// Tony

On 04/16/2014 10:54 AM, Boszormenyi Zoltan wrote:
> 2014-01-18 18:08 keltezéssel, Boszormenyi Zoltan írta:
>> Hi,
>>
>>
>> Alvaro Herrera wrote:
>>> Boszormenyi Zoltan escribió:
>>>> Rebased patches after the regression test and other details were fixed
>>>> in the infrastructure part.
>>>
>>> This thread started in 2010, and various pieces have been applied
>>> already and some others have changed in nature. Would you please post a
>>> new patchset, containing rebased patches that still need application, in
>>> a new email thread to be linked in the commitfest entry?
>>
>> I hope Thunderbird did the right thing and didn't include the reference
>> message ID when I told it to "edit as new". So supposedly this is a new
>> thread but with all the cc: addresses kept.
>>
>> I have rebased all remaining patches and kept the numbering.
>> All the patches from 18 to 25 that were previously in the
>> "ECPG infrastructure" CF link are included here.
>>
>> There is no functional change.
>
> Because of the recent bugfixes in the ECPG area, the patchset
> didn't apply cleanly anymore. Rebased with no functional change.
>
> Best regards,
> Zoltán Böszörményi
>