On Thu, Mar 23, 2017 at 9:21 AM, Dan Kennedy <danielk1...@gmail.com> 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

Reply via email to