Re: [sqlite] Possibly pointless assert

[Scott Robison]

On Thu, Mar 23, 2017 at 11:05 AM, Dan Kennedy  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-317.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
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Possibly pointless assert

[Dan Kennedy]

On 03/23/2017 11:46 PM, Scott Robison wrote:

On Thu, Mar 23, 2017 at 10:17 AM, Scott Robison  wrote:

On Thu, Mar 23, 2017 at 9:21 AM, Dan Kennedy  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-317.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
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Possibly pointless assert

[Scott Robison]

On Thu, Mar 23, 2017 at 10:17 AM, Scott Robison  wrote:
> On Thu, Mar 23, 2017 at 9:21 AM, Dan Kennedy  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-317.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  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
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Possibly pointless assert

[Scott Robison]

On Thu, Mar 23, 2017 at 9:21 AM, Dan Kennedy  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
> sqlite-users@mailinglists.sqlite.org
> http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users



-- 
Scott Robison
___
sqlite-users mailing list
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users

Re: [sqlite] Possibly pointless assert

[Dan Kennedy]

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
sqlite-users@mailinglists.sqlite.org
http://mailinglists.sqlite.org/cgi-bin/mailman/listinfo/sqlite-users