Re: Review: [HACKERS] ECPG infrastructure changes part 1, was: Re: ECPG fixes
2014-01-16 22:13 keltezéssel, Alvaro Herrera írta: Alvaro Herrera escribió: Boszormenyi Zoltan escribió: All patches are attached again for completeness. Thanks. I pushed a commit comprising patches 09 through 14. Now also pushed 15 to 17. Thanks very much. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Review: [HACKERS] ECPG infrastructure changes part 1, was: Re: ECPG fixes
Boszormenyi Zoltan escribió: All patches are attached again for completeness. Thanks. I pushed a commit comprising patches 09 through 14. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Review: [HACKERS] ECPG infrastructure changes part 1, was: Re: ECPG fixes
Alvaro Herrera escribió: Boszormenyi Zoltan escribió: All patches are attached again for completeness. Thanks. I pushed a commit comprising patches 09 through 14. Now also pushed 15 to 17. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Review: [HACKERS] ECPG infrastructure changes part 1, was: Re: ECPG fixes
On 12/6/13, 9:58 AM, Antonin Houska wrote: Tested git apply and build again. No warnings. The regression test also looks good to me now. I'm done with this review. (Not sure if I should move it to 'ready for committer' status or the CFM should do). You should do that, but I'll do it now. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Review: [HACKERS] ECPG infrastructure changes part 1, was: Re: ECPG fixes
Tested git apply and build again. No warnings. The regression test also looks good to me now. I'm done with this review. (Not sure if I should move it to 'ready for committer' status or the CFM should do). // Antonin Houska (Tony) On 12/06/2013 02:01 PM, Boszormenyi Zoltan wrote: 2013-12-04 14:51 keltezéssel, Boszormenyi Zoltan írta: 2013-12-03 16:48 keltezéssel, Antonin Houska írta: 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. I have already answered this in my previous answer. And I was wrong in that. The comments in the test were rearranged a little and the fact in the above comment is now actually tested. Some harmless unused variables were also removed and an uninitialized variable usage was fixed. Because of these and the above changes a lot of patches need to be rebased. All patches are attached again for completeness. 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
Review: [HACKERS] ECPG infrastructure changes part 1, was: Re: ECPG fixes
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 string will 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Review: [HACKERS] ECPG infrastructure changes part 1, was: Re: ECPG fixes
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/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
