Re: Fix a Oracle-compatible　instr function　in the documentation

Tatsuo Ishii <ishii@sraoss.co.jp> writes:
>> The documentation of PL/pgSQL provides sample codes of Oracle-compatible
>> instr functions. However, the behaviour is a little differet.
>> Oracle's instr raises an error when the forth argument value is less than
>> zero, but the sample code returns zero. This patch fixes this.

> Your patch fixes only instr(string varchar, string_to_search varchar, beg_index integer).
> However in the doc there is another instr(string varchar, string_to_search varchar, beg_index integer, occur_index
integer).

> Shouldn't this be fixed as well?

Nagata-san's proposed addition makes no sense in the second instr()
implementation, which has no occur_index parameter; it only makes sense
in the third one.  I think the submitted patch is probably against some
old version of plpgsql.sgml and the line numbers in it are confusing
"patch" into thinking it should go into the second instr() implementation.

I checked with http://rextester.com/l/oracle_online_compiler
and verified that Oracle throws an error for zero/negative occur_index,
so the patch is definitely correct as far as it goes.  However, poking
at it a bit further, there is another problem: our sample code disagrees
with Oracle about what negative beg_index means.  For example, our code
produces

regression=# select instr('foo bar baz bar quux', 'bar', -6) ;
 instr
-------
    13
(1 row)

regression=# select instr('foo bar baz bar quux', 'bar', -7) ;
 instr
-------
     5
(1 row)

whereas I get this with Oracle:

select instr('foo bar baz bar quux', 'bar', -6) from dual
13
select instr('foo bar baz bar quux', 'bar', -7) from dual
13
select instr('foo bar baz bar quux', 'bar', -8) from dual
13
select instr('foo bar baz bar quux', 'bar', -9) from dual
5

Evidently, they consider that negative beg_index indicates
the last place where the target substring can *begin*, whereas
our code thinks it is the last place where the target can *end*.

After a bit of fooling around with it, I produced code that agrees
with Oracle as far as I can tell, but it wouldn't be a bad idea
for someone else to test it some more.

I eliminated a couple of obvious ineffiencies while at it, notably
using position() for what could be a simple equality test in the
backwards-search loops.  I also changed the header comment, which
was both very incomplete and wrong in detail.

While the response to negative occur_index is probably not that
interesting in the field, the non-equivalent behavior for negative
beg_index is very definitely something that could bite people doing
Oracle translations.  So I agree we'd better back-patch this, and
maybe even call it out as a bug fix in the release notes.

            regards, tom lane

diff --git a/doc/src/sgml/plpgsql.sgml b/doc/src/sgml/plpgsql.sgml
index 7d23ed4..9d9e362 100644
*** a/doc/src/sgml/plpgsql.sgml
--- b/doc/src/sgml/plpgsql.sgml
*************** $$ LANGUAGE plpgsql STRICT IMMUTABLE;
*** 5650,5668 ****
  <programlisting>
  --
  -- instr functions that mimic Oracle's counterpart
! -- Syntax: instr(string1, string2, [n], [m]) where [] denotes optional parameters.
  --
! -- Searches string1 beginning at the nth character for the mth occurrence
! -- of string2.  If n is negative, search backwards.  If m is not passed,
! -- assume 1 (search starts at first character).
  --

  CREATE FUNCTION instr(varchar, varchar) RETURNS integer AS $$
- DECLARE
-     pos integer;
  BEGIN
!     pos:= instr($1, $2, 1);
!     RETURN pos;
  END;
  $$ LANGUAGE plpgsql STRICT IMMUTABLE;

--- 5650,5668 ----
  <programlisting>
  --
  -- instr functions that mimic Oracle's counterpart
! -- Syntax: instr(string1, string2 [, n [, m]]) where [] denotes optional parameters.
  --
! -- Search string1, beginning at the nth character, for the mth occurrence
! -- of string2.  If n is negative, search backwards, starting at the abs(n)'th
! -- character from the end of string1.
! -- If n is not passed, assume 1 (search starts at first character).
! -- If m is not passed, assume 1 (find first occurrence).
! -- Returns starting index of string2 in string1, or 0 if string2 is not found.
  --

  CREATE FUNCTION instr(varchar, varchar) RETURNS integer AS $$
  BEGIN
!     RETURN instr($1, $2, 1);
  END;
  $$ LANGUAGE plpgsql STRICT IMMUTABLE;

*************** BEGIN
*** 5688,5700 ****
      ELSIF beg_index < 0 THEN
          ss_length := char_length(string_to_search);
          length := char_length(string);
!         beg := length + beg_index - ss_length + 2;

          WHILE beg > 0 LOOP
              temp_str := substring(string FROM beg FOR ss_length);
!             pos := position(string_to_search IN temp_str);
!
!             IF pos > 0 THEN
                  RETURN beg;
              END IF;

--- 5688,5698 ----
      ELSIF beg_index < 0 THEN
          ss_length := char_length(string_to_search);
          length := char_length(string);
!         beg := length + 1 + beg_index;

          WHILE beg > 0 LOOP
              temp_str := substring(string FROM beg FOR ss_length);
!             IF string_to_search = temp_str THEN
                  RETURN beg;
              END IF;

*************** DECLARE
*** 5721,5759 ****
      length integer;
      ss_length integer;
  BEGIN
!     IF beg_index > 0 THEN
!         beg := beg_index;
!         temp_str := substring(string FROM beg_index);

          FOR i IN 1..occur_index LOOP
              pos := position(string_to_search IN temp_str);
!
!             IF i = 1 THEN
!                 beg := beg + pos - 1;
!             ELSE
!                 beg := beg + pos;
              END IF;
!
!             temp_str := substring(string FROM beg + 1);
          END LOOP;

!         IF pos = 0 THEN
!             RETURN 0;
!         ELSE
!             RETURN beg;
!         END IF;
      ELSIF beg_index < 0 THEN
          ss_length := char_length(string_to_search);
          length := char_length(string);
!         beg := length + beg_index - ss_length + 2;

          WHILE beg > 0 LOOP
              temp_str := substring(string FROM beg FOR ss_length);
!             pos := position(string_to_search IN temp_str);
!
!             IF pos > 0 THEN
                  occur_number := occur_number + 1;
-
                  IF occur_number = occur_index THEN
                      RETURN beg;
                  END IF;
--- 5719,5750 ----
      length integer;
      ss_length integer;
  BEGIN
!     IF occur_index <= 0 THEN
!         RAISE 'argument ''%'' is out of range', occur_index
!           USING ERRCODE = '22003';
!     END IF;

+     IF beg_index > 0 THEN
+         beg := beg_index - 1;
          FOR i IN 1..occur_index LOOP
+             temp_str := substring(string FROM beg + 1);
              pos := position(string_to_search IN temp_str);
!             IF pos = 0 THEN
!                 RETURN 0;
              END IF;
!             beg := beg + pos;
          END LOOP;

!         RETURN beg;
      ELSIF beg_index < 0 THEN
          ss_length := char_length(string_to_search);
          length := char_length(string);
!         beg := length + 1 + beg_index;

          WHILE beg > 0 LOOP
              temp_str := substring(string FROM beg FOR ss_length);
!             IF string_to_search = temp_str THEN
                  occur_number := occur_number + 1;
                  IF occur_number = occur_index THEN
                      RETURN beg;
                  END IF;