Re: Review: ECPG FETCH readahead

Hi,

thanks for the review.

2014-04-23 17:20 keltezéssel, Antonin Houska írta:
> 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?

It makes sense to add one.

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

All exported ECPG functions returns bool. IIRC the code generated by
"EXEC SQL WHENEVER <something-else-than-CONTINUE>" makes use
of the returned value.

>
> * line breaks (pgindent might help):
>
> static void
> output_cursor_name(struct cursor *ptr)
> {
>
> instead of
>
> static void output_cursor_name(struct cursor *ptr)
> {

OK.

>
>
> * 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".

Yes, oversight.

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

What do you mean exactly?

>
> 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 caching code tries to do the correct thing whichever direction
the cursor is scanned. AFAIR this one explicitly tests invalidating
the readahead window if you fall off it by using MOVE FORWARD ALL.

>
> 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 :-)

B-)

> * 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.

Oh. Erm. That is a leftover from a previous incarnation when
I thought it's a good idea to let the server return the actual
scrollability flag because the server can actually know.
The simple query when running in psql is implicitly scrollable
but the query with the join is not. That idea was shot down
with prejudice but I forgot to adjust the test and remove
one of these cursors. Now all queries where scrollability is
not specified are explicitly modified (behind the application's
back) to have NO SCROLL. ( Please, don't revise your previous
opinion, Tom Lane included... ;-) )

>
>
> * 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.

When I re-roll the patchset, I will move this chunk to 26.

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

Contents of tuples ... reflect ...

Plural. The verb is correct as is. :-)

> 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

I'll re-read and see what context "need" is in. Hey, a context diff
would have made it more obvious. ;-)

> 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

Sure. This sentence is not about "I" but it's true either way. :-D

> 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

I will fix these.

>
>
> 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 :-)

I will read your comments again with fresh eyes.

>
> That's it for my review.

Thank you very much.

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