284598 - INSTR function is not supported by postgres

Status: RESOLVED FIXED

(Bugzilla :: Bugzilla-General, defect)

Description

[Tomas Kopal]

INSTR function for finding whether substring is part of other string is not
supported by postgres. Both MySQL and Postgres do support POSITION functions,
but it looks that POSITION is in turn not supported by Oracle.
So lets create a new DB function for case sensitive search of substring within a
string.
Why case sensitive? It does correspond to how MySQL v3 and Postgres works, MySQL
newer than 4.0.1 does case insensitive search, but we are already using LOWER
with this function so case sensitive makes more sense. Also it is posible to use
case sensitive function for case insensitive search using LOWER, but not vice versa.

Comment 1

[Max Kanat-Alexander]

At this point I'd rather just use POSITION, and then change that to a
Bugzilla::DB function when we do Oracle support.

Comment 2

[Tomas Kopal]

That's ok with me, I just tought that when we are at that, we can do it in one
shot :-).

Comment 3

[Tomas Kopal]

Hmmm, as I am thinking about it again, I don't think it will work. We need at
least at one place in Search.pm to do case-sensitive substring, and POSITION is
case insensitive on MySQL. The solution is type cast the parameters to BINARY,
but that breaks postgres (Pg does not have BINARY type). So I think having DB
method would be better solution.

Comment 4

[Max Kanat-Alexander]

The ANSI equivalent of INSTR is SUBSTRING, not POSITION. :-)

Comment 5

[Max Kanat-Alexander]

Oh nevermind, I'm wrong, you're right. It's POSITION. :-)

And you're right. There's no standard case-sensitive search function in MySQL.
That's really quite stupid.

So we need a sql_position function, you're right, which does a case-sensitive
search, and then we can explicitly call LOWER() when we want a case-insensitive
search.

This will pobably fix strange Bugzilla bugs while we do it, anyhow. :-)

Comment 6

[Tomas Kopal]

Attachment: V1

Here is the patch. It's tested on MySQL only.

Comment 7

[Max Kanat-Alexander]

Comment on attachment 176248 [details] [diff] [review]
V1

>+                             "WHERE " . $dbh->sql_position('/', 'filename') .
>+                             " OR " . $dbh->sql_position('\\\\', 'filename'));

  That looks like it will produce:

  POSITION(/ IN filename)

  for PostgreSQL. Note the lack of quoting. I think that if we want to pass a
literal to the function, we should do q{'\'}. I'd like the functions to be as
flexible as possible, and not have them do their own quoting. You'll want to
note that arguments aren't quoted, though.

  You might also want to change the one call to POSITION in checksetup, for the
heck of it, even though it doesn't need it.

>+            $dbh->sql_position(lc(::SqlQuote($email)), "LOWER(login_name)") .

  That looks like it should be &::SqlQuote($email).

>+            push(@list, $dbh->sql_position(lc(::SqlQuote($word)),
>+                                           "LOWER($field)"));

  Same there. Why did you remove the &?

Comment 8

[Tomas Kopal]

Attachment: V2

(In reply to comment #7)
> (From update of attachment 176248 [details] [diff] [review] [edit])
> >+				 "WHERE " . $dbh->sql_position('/', 'filename')
.
> >+				 " OR " . $dbh->sql_position('\\\\',
'filename'));
> 
>   That looks like it will produce:
> 
>   POSITION(/ IN filename)
> 
>   for PostgreSQL. Note the lack of quoting. I think that if we want to pass a

> literal to the function, we should do q{'\'}. I'd like the functions to be as

> flexible as possible, and not have them do their own quoting. You'll want to
> note that arguments aren't quoted, though.

Oooops :-). Thanks, good one. I originally didn't change checksetup, as this
code never gets called for postgres (we don't have old pg databases to
upgrade), but I changed my mind just before posting the patch. And I screwed it
up :-).

> 
>   You might also want to change the one call to POSITION in checksetup, for
the
> heck of it, even though it doesn't need it.

Good point, fixed.

> 
> >+		$dbh->sql_position(lc(::SqlQuote($email)), "LOWER(login_name)")
.
> 
>   That looks like it should be &::SqlQuote($email).
> 
> >+		push(@list, $dbh->sql_position(lc(::SqlQuote($word)),
> >+					       "LOWER($field)"));
> 
>   Same there. Why did you remove the &?
> 

Cleanup, simplification, it's not neccessary. From Perl 5 decumentation: 

-------
To call subroutines:
     NAME(LIST); # & is optional with parentheses.
-------
and
-------
A subroutine may be called using the "&" prefix. The "&" is optional in modern
Perls, and so are the parentheses if the subroutine has been predeclared.
(Note, however, that the "&" is NOT optional when you re just naming the
subroutine, such as when it s used as an argument to defined() or undef(). Nor
is it optional when you want to do an indirect subroutine call with a
subroutine name or reference using the &$subref() or &{$subref}() constructs.
See perlref for more on that.)

Comment 9

[Max Kanat-Alexander]

Comment on attachment 176322 [details] [diff] [review]
V2

Good. Except that you need to note the stuff about how literals should be
passed, in the docs.

Comment 10

[Tomas Kopal]

Attachment: V2.1

Added note about quoting string constants, no other change.

Comment 11

[Max Kanat-Alexander]

Checking in checksetup.pl;
/cvsroot/mozilla/webtools/bugzilla/checksetup.pl,v  <--  checksetup.pl
new revision: 1.360; previous revision: 1.359
done
Checking in Bugzilla/DB.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB.pm,v  <--  DB.pm
new revision: 1.25; previous revision: 1.24
done
Checking in Bugzilla/Search.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/Search.pm,v  <--  Search.pm
new revision: 1.87; previous revision: 1.86
done
Checking in Bugzilla/User.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/User.pm,v  <--  User.pm
new revision: 1.43; previous revision: 1.42
done
Checking in Bugzilla/DB/Mysql.pm;
/cvsroot/mozilla/webtools/bugzilla/Bugzilla/DB/Mysql.pm,v  <--  Mysql.pm
new revision: 1.3; previous revision: 1.2
done