Quantcast

Possibly pointless assert

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Possibly pointless assert

Scott Robison-2
Take a look at http://www.sqlite.org/cgi/src/artifact/3ed64afc49c0a222?ln=2214,2233
(especially the assert within).

I may not be understanding something, but that assert seems pointless
to me. The point of the loop is to check all the columns in an index
to see if they are all binary collated. If any column is not binary
collated, then exit early, which will skip the following if statement
at 2234.

It feels to me like that assert was added as a mid-development sanity
check when it was being developed against a known database. I had it
trip on me today unexpectedly.

If I am incorrect and that is a useful assertion, I'd like to
understand the reason why. Otherwise, the if statement at 2232 does
everything the assert at 2230 does, making the assert fire when the
code is working correctly.

--
Scott Robison
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Possibly pointless assert

Dan Kennedy-4
On 03/23/2017 04:45 AM, Scott Robison wrote:
> Take a look at http://www.sqlite.org/cgi/src/artifact/3ed64afc49c0a222?ln=2214,2233
> (especially the assert within).
>
> I may not be understanding something, but that assert seems pointless
> to me.


The assert() says that if the buffer pointed to by Column.zColl contains
the string "BINARY", then it must point actually point to global buffer
sqlite3StrBINARY, not to some other buffer that contains the same bytes.
i.e. the assert() means that the next line could be written as:

   if( sqlite3StrBINARY==zColl ) break;

instead of:

   if( sqlite3_stricmp(sqlite3StrBINARY, zColl) ) break;

I think it's likely an oversight that that line was not rewritten. Or
perhaps just a choice to take the safer option in case the assert() is
not actually true. There is another part of the code where that
assumption is made and the (sqlite3StrBINARY==zColl) comparison is used,
although I think it's just an optimization - SQLite should not return
the wrong answer even if the assert() can be false.

How did you trip the assert()? i.e. what is the database schema and
query that cause it to fail?

Dan.







> The point of the loop is to check all the columns in an index
> to see if they are all binary collated. If any column is not binary
> collated, then exit early, which will skip the following if statement
> at 2234.
>
> It feels to me like that assert was added as a mid-development sanity
> check when it was being developed against a known database. I had it
> trip on me today unexpectedly.
>
> If I am incorrect and that is a useful assertion, I'd like to
> understand the reason why. Otherwise, the if statement at 2232 does
> everything the assert at 2230 does, making the assert fire when the
> code is working correctly.
>

_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Possibly pointless assert

Scott Robison-2
On Thu, Mar 23, 2017 at 9:21 AM, Dan Kennedy <[hidden email]> wrote:

> On 03/23/2017 04:45 AM, Scott Robison wrote:
>>
>> Take a look at
>> http://www.sqlite.org/cgi/src/artifact/3ed64afc49c0a222?ln=2214,2233
>> (especially the assert within).
>>
>> I may not be understanding something, but that assert seems pointless
>> to me.
>
>
>
> The assert() says that if the buffer pointed to by Column.zColl contains the
> string "BINARY", then it must point actually point to global buffer
> sqlite3StrBINARY, not to some other buffer that contains the same bytes.
> i.e. the assert() means that the next line could be written as:
>
>   if( sqlite3StrBINARY==zColl ) break;
>
> instead of:
>
>   if( sqlite3_stricmp(sqlite3StrBINARY, zColl) ) break;
>
> I think it's likely an oversight that that line was not rewritten. Or
> perhaps just a choice to take the safer option in case the assert() is not
> actually true. There is another part of the code where that assumption is
> made and the (sqlite3StrBINARY==zColl) comparison is used, although I think
> it's just an optimization - SQLite should not return the wrong answer even
> if the assert() can be false.

Correct. If the line following was just using an equality comparison
of two pointers, the assert would make a bit more sense. Given that
the following comparison uses the "safer" stricmp, the assertion seems
pointless. It winds up asserting something is wrong when in fact the
code works exactly as advertised.

>
> How did you trip the assert()? i.e. what is the database schema and query
> that cause it to fail?

In trying to track down issues recently, a team member defined
SQLITE_DEBUG. My "fix" was to simply undefine SQLITE_DEBUG, thus
compiling out the assertions anyway. Since we won't have it in
production code, I wouldn't call it a bug, just an over enthusiastic
bit of error prevention.

The query was apparently a vacuum. I'll synthesize a test case and
submit it later.

>
> Dan.
>
>
>
>
>
>
>
>> The point of the loop is to check all the columns in an index
>> to see if they are all binary collated. If any column is not binary
>> collated, then exit early, which will skip the following if statement
>> at 2234.
>>
>> It feels to me like that assert was added as a mid-development sanity
>> check when it was being developed against a known database. I had it
>> trip on me today unexpectedly.
>>
>> If I am incorrect and that is a useful assertion, I'd like to
>> understand the reason why. Otherwise, the if statement at 2232 does
>> everything the assert at 2230 does, making the assert fire when the
>> code is working correctly.
>>
>
> _______________________________________________
> sqlite-users mailing list
> [hidden email]
> http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users



--
Scott Robison
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Possibly pointless assert

Scott Robison-2
On Thu, Mar 23, 2017 at 10:17 AM, Scott Robison <[hidden email]> wrote:

> On Thu, Mar 23, 2017 at 9:21 AM, Dan Kennedy <[hidden email]> wrote:
>> How did you trip the assert()? i.e. what is the database schema and query
>> that cause it to fail?
>
> In trying to track down issues recently, a team member defined
> SQLITE_DEBUG. My "fix" was to simply undefine SQLITE_DEBUG, thus
> compiling out the assertions anyway. Since we won't have it in
> production code, I wouldn't call it a bug, just an over enthusiastic
> bit of error prevention.
>
> The query was apparently a vacuum. I'll synthesize a test case and
> submit it later.

Note: I'm on Windows 10 and reproduced this with the amalgamation
downloaded today from
http://sqlite.com/2017/sqlite-amalgamation-3170000.zip

Step 1: Using sqlite3 shell, created a database test.db with the
following schema:

    CREATE TABLE a(b text collate binary, c text collate nocase);
    CREATE INDEX ab on a(b);
    CREATE INDEX ac on a(c);

Note: I did not insert any data. It is not necessary.

Step 2: Copied the 35 line sample C code from
http://sqlite.com/quickstart.html into sqlite-assert-test.c. My only
change was to change the sqlite3.h include from <sqlite3.h> to
"sqlite3.h"

Step 3: Extracted the amalgamation files into the directory with
sqlite-assert-test.c.

Step 4: Opened a 64 bit native build command prompt from Visual C++ 2015.

Step 5: Build the test program as follows:

    cl /Zi -DSQLITE_DEBUG sqlite-assert-test.c sqlite3.c

Step 6: Ran the resulting executable as:

    sqlite-assert-test.exe test.db vacuum

It throws an assertion, presumably trying to vacuum the ab index.
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Possibly pointless assert

Dan Kennedy-4
On 03/23/2017 11:46 PM, Scott Robison wrote:

> On Thu, Mar 23, 2017 at 10:17 AM, Scott Robison <[hidden email]> wrote:
>> On Thu, Mar 23, 2017 at 9:21 AM, Dan Kennedy <[hidden email]> wrote:
>>> How did you trip the assert()? i.e. what is the database schema and query
>>> that cause it to fail?
>> In trying to track down issues recently, a team member defined
>> SQLITE_DEBUG. My "fix" was to simply undefine SQLITE_DEBUG, thus
>> compiling out the assertions anyway. Since we won't have it in
>> production code, I wouldn't call it a bug, just an over enthusiastic
>> bit of error prevention.
>>
>> The query was apparently a vacuum. I'll synthesize a test case and
>> submit it later.
> Note: I'm on Windows 10 and reproduced this with the amalgamation
> downloaded today from
> http://sqlite.com/2017/sqlite-amalgamation-3170000.zip
>
> Step 1: Using sqlite3 shell, created a database test.db with the
> following schema:
>
>      CREATE TABLE a(b text collate binary, c text collate nocase);
>      CREATE INDEX ab on a(b);
>      CREATE INDEX ac on a(c);

Thanks for reporting this. Removed the assert() here:

   http://www.sqlite.org/src/info/9f2e04d3c3526b5f

Dan.

_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: Possibly pointless assert

Scott Robison-2
On Thu, Mar 23, 2017 at 11:05 AM, Dan Kennedy <[hidden email]> wrote:

> On 03/23/2017 11:46 PM, Scott Robison wrote:
>>
>> Note: I'm on Windows 10 and reproduced this with the amalgamation
>> downloaded today from
>> http://sqlite.com/2017/sqlite-amalgamation-3170000.zip
>>
>> Step 1: Using sqlite3 shell, created a database test.db with the
>> following schema:
>>
>>      CREATE TABLE a(b text collate binary, c text collate nocase);
>>      CREATE INDEX ab on a(b);
>>      CREATE INDEX ac on a(c);
>
>
> Thanks for reporting this. Removed the assert() here:
>
>   http://www.sqlite.org/src/info/9f2e04d3c3526b5f

Glad to do it. Complete fluke that I came across it. Still, nice to
"contribute" something, even if this trivial. :)

--
Scott Robison
_______________________________________________
sqlite-users mailing list
[hidden email]
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users
Loading...