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.7010...@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.9020...@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 > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
