Re: Review: ECPG FETCH readahead

2014-04-24 15:19 keltezéssel, Boszormenyi Zoltan írta:
> 2014-04-24 14:50 keltezéssel, Michael Meskes írta:
>> Thanks an awful lot Antonin.
>>
>>> Committer availability might well be the issue, but missing review
>>> probably too.
>> Yes, you're right. If my taks is mostly one last glance and a commit I will make time
>> for that.
>>
>>> Whether this review is enough to move the patch to "ready for committer"
>>> - I tend to let the next CFM decide. (I don't find it productive to
>>> ignite another round of discussion about kinds of reviews - already saw
>>> some.)
>> I saw some remarks in your review that Zoltan wants to address. Once I got the
>> updated version I'll have a look at it.
>>
>> Zoltan, could you send a new version by end of day tomorrow? I'll be sitting on
>> a plane for a longer time again on Saturday. :)
>
> I will try to.

Unfortunately, I won't make the deadline because of life
(I had to attend a funeral today) and because Antonin has
opened a can of worms with this comment:

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

There are some aspects that may need a new discussion.

The SQL standard wants an "updatable cursor" for positioned DML
(i.e. UPDATE/DELETE with the WHERE CURRENT OF clause)
This means passing FOR UPDATE in the query.

FOR UPDATE + SCROLL cursor is an impossible combination,
ERROR is thrown when DECLARE is executed. This combination can
(and should?) be detected in the ECPG preprocessor and it would
prevent runtime errors. It's not implemented at the moment.

Fortunately, a previous discussion resulted in explicitly passing
NO SCROLL for cursors where SCROLL is not specified, it's in 25.patch

I intend to extend it a little for SQL standard compliance with
embedded SQL. FOR UPDATE should also implicitly mean NO SCROLL.
Both the FOR UPDATE + explicit SCROLL and the explicit SCROLL +
usage of positioned DML can be detected in the preprocessor and
they should throw an error. Then the regression test would really make
sense.

But these checks in ECPG would still leave a big hole, and it's the other
DECLARE variant with the query passed in a prepared statement with
"EXEC SQL PREPARE prep_stmt FROM :query_string;"

Plugging this hole would require adding a simplified syntax checker to
libecpg that only checks the SelectStmt or re-adding the backend code
to tell the application the cursor's scrollable (and perhaps the updatable)
property.

I must have forgotten but surely this was the reason for changing the
DECLARE command tag in the first place which was shot down already.
So, only the other choice remains, the syntax checker in ecpglib.

I think implementing it would make the caching code miss 9.4, since
it adds a whole new set of code but the Perl magic for the ECPG syntax
checker may be mostly reusable here.

Best regards,
Zoltán Böszörményi