View Issue Details Jump to Notes ] Issue History ] Print ]
IDProjectCategoryView StatusDate SubmittedLast Update
0019343FPCDatabase Componentspublic2011-05-12 09:282013-07-22 07:06
ReporterLacaK 
Assigned ToJoost van der Sluis 
PrioritynormalSeveritytrivialReproducibilityhave not tried
StatusclosedResolutionno change required 
PlatformOSOS Version
Product Version2.4.3Product Build 
Target VersionFixed in Version 
Summary0019343: UnPrepare should be called when Prepared in procedure TCustomSQLQuery.InternalClose
DescriptionThere is IMHO reversed logic on lines:

1151 if (not IsPrepared) and (assigned(database)) and (assigned(FCursor)) then TSQLConnection(database).UnPrepareStatement(FCursor);

1411 if (not IsPrepared) and (assigned(database)) and (assigned(FCursor)) then TSQLConnection(database).UnPrepareStatement(Fcursor);


UnPrepare should be called when statement *IsPrepared* not when *not IsPrepared*
TagsNo tags attached.
FPCOldBugId
Fixed in Revision
Attached Files

- Relationships

-  Notes
(0048230)
Joost van der Sluis (developer)
2011-05-12 10:43
edited on: 2011-05-12 10:46

Try it and run the full testsuite. Getting this right was quite a puzzle. You can also use 'svn blame' and read the commit-messages. Maybe they give some more information.

And, if this is a bug, you can provide an example which shows that something goes wrong. Looking at code only, spotting a bug without being able to show the problem, doesn't make it a bug. (IsPrepared does not what you think it does)

So, please show me what the actual problem is. I don't see any right now.

(0048414)
LacaK (reporter)
2011-05-19 15:05
edited on: 2011-05-20 08:24

Please look at this revision (there was introduced (not IsPrepared)): http://svn.freepascal.org/cgi-bin/viewvc.cgi/trunk/packages/fcl-db/src/sqldb/sqldb.pp?r1=12284&r2=12427 [^]

I can show problem on ExecSQL method, where was used ExecDidPrepare variable, which saved state of cursor *BEFORE* Prepare was called.
(It is clear, that state changes after call to Prepare to IsPrepared)
And this variable was used after call to Execute ... if cursor was prepared before entering ExecSQL method then leave it prepared else unprepare.

In current implementation unprepare is never called because cursor is always prepared (thanks to unconditional call to Prepare).
So now when leaving ExecSQL, cursor always remains prepared, regardless of whether prior was Prepared or not.

So we must say what is purpose of this command?:
    if (not IsPrepared) and (assigned(database)) and (assigned(FCursor)) then TSQLConnection(database).UnPrepareStatement(Fcursor);

1. original purpose was unprepare after execute if was not prepared before ... if we want this then we must change code
2. or another meaning can be unprepare if some exceptions occurs during prepare call (but what unprepare if prepare does not completed) ... if we wan this then we must change at least comment lines to reflect this

(0051409)
Marco van de Voort (manager)
2011-09-02 23:27

bump
(0059250)
Marco van de Voort (manager)
2012-05-05 11:47

Bump with feedback, year old again.
(0059482)
LacaK (reporter)
2012-05-11 14:49
edited on: 2012-05-11 15:03

I ran test suite with "not IsPrepared" changed to "IsPrepared" for SQLite, Firebird and MSSQL.

For Firebird there appears failure:
<test name="TTestFieldTypes.TestRowsAffected">
<failure ExceptionClassName="EAssertionFailedError">
<message> expected: <1> but was: <-1></message>
</failure>
</test>
It is because after ExecSQL, statement is unprepared and when it is once unprepared we can not get "records affected" of unprepared statement.

So we can not change it as easy as I wrote.
But question remains: what is purpose of this command:

    if (not IsPrepared) and (assigned(database)) and (assigned(FCursor)) then TSQLConnection(database).UnPrepareStatement(Fcursor);

I read it as "unprepare" when is "unprepared".
I do not see logic in it ;-)

When I comment it in procedure TCustomSQLQuery.ExecSQL and change "not IsPrepared" to "IsPrepared" in procedure TCustomSQLQuery.InternalClose then tests pass ok and also it seems to me as logical solution ;-)

(0059574)
LacaK (reporter)
2012-05-14 07:27

Thinking more about subject, there can be also other interpretation.
When error occurs during preparation stage, then IsPrepared is false.

So testing if (not Prepared) can be read as "if something went wrong during preparation" then do "clean up" by calling UnPrepare.

If this is intention, then adding comment will be sufficient and we can close this bug report.
(0068810)
LacaK (reporter)
2013-07-12 07:13

Please close as "No change required"
(0068811)
Reinier Olislagers (developer)
2013-07-12 11:26

Resolving as requested; please close.

- Issue History
Date Modified Username Field Change
2011-05-12 09:28 LacaK New Issue
2011-05-12 09:28 LacaK Status new => assigned
2011-05-12 09:28 LacaK Assigned To => Joost van der Sluis
2011-05-12 10:43 Joost van der Sluis Note Added: 0048230
2011-05-12 10:46 Joost van der Sluis Note Edited: 0048230
2011-05-19 15:05 LacaK Note Added: 0048414
2011-05-20 08:24 LacaK Note Edited: 0048414
2011-09-02 23:27 Marco van de Voort Note Added: 0051409
2012-05-05 11:47 Marco van de Voort Note Added: 0059250
2012-05-05 11:47 Marco van de Voort Status assigned => feedback
2012-05-11 14:49 LacaK Note Added: 0059482
2012-05-11 15:03 LacaK Note Edited: 0059482
2012-05-14 07:27 LacaK Note Added: 0059574
2013-07-12 07:13 LacaK Note Added: 0068810
2013-07-12 07:13 LacaK Status feedback => assigned
2013-07-12 11:26 Reinier Olislagers Note Added: 0068811
2013-07-12 11:26 Reinier Olislagers Status assigned => resolved
2013-07-12 11:26 Reinier Olislagers Resolution open => no change required
2013-07-22 07:06 LacaK Status resolved => closed



MantisBT 1.2.12[^]
Copyright © 2000 - 2012 MantisBT Group
Powered by Mantis Bugtracker