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

Fix a Oracle-compatible instr function in the documentation

From
Yugo Nagata
Date:
Hi,

Attached is a patch to fix a very trivial issue of the documentation.

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.

Regards,

-- 
Yugo Nagata <nagata@sraoss.co.jp>

Attachment

Re: Fix a Oracle-compatible instr function in thedocumentation

From
Tatsuo Ishii
Date:
> Hi,
> 
> Attached is a patch to fix a very trivial issue of the documentation.
> 
> 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.

Do we need treat this as a bug fix? If so, do we need to back patch as
well?

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


Re: Fix a Oracle-compatible instr function in thedocumentation

From
Tatsuo Ishii
Date:
>> Hi,
>> 
>> Attached is a patch to fix a very trivial issue of the documentation.
>> 
>> 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.
> 
> Do we need treat this as a bug fix? If so, do we need to back patch as
> well?

I have added this to CF 2018-01.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


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

From
Yugo Nagata
Date:
On Sun, 31 Dec 2017 17:52:49 +0900 (JST)
Tatsuo Ishii <ishii@sraoss.co.jp> wrote:

> >> Hi,
> >> 
> >> Attached is a patch to fix a very trivial issue of the documentation.
> >> 
> >> 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.
> > 
> > Do we need treat this as a bug fix? If so, do we need to back patch as
> > well?

This is a little improvement of the documentation to reduce confusion of users
who work on migration from Oracle. I don't know whether this need tobe back
patched, so I'll leave a decision up to commiters.

> 
> I have added this to CF 2018-01.

Thank you.

> 
> Best regards,
> --
> Tatsuo Ishii
> SRA OSS, Inc. Japan
> English: http://www.sraoss.co.jp/index_en.php
> Japanese:http://www.sraoss.co.jp


-- 
Yugo Nagata <nagata@sraoss.co.jp>


Re: Fix a Oracle-compatible instr function in thedocumentation

From
Tatsuo Ishii
Date:
> 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?

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp


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

From
Tom Lane
Date:
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;

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

From
Tom Lane
Date:
I wrote:
> 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 spent some more time comparing this version's behavior with
rextester's Oracle instance, and couldn't find any other
discrepancies, so I pushed it.

            regards, tom lane


Re: Fix a Oracle-compatible instr function in thedocumentation

From
Tatsuo Ishii
Date:
> I spent some more time comparing this version's behavior with
> rextester's Oracle instance, and couldn't find any other
> discrepancies, so I pushed it.

Great!

I agree with your commit message:

> Back-patch to all supported branches.  Although this patch only touches
> documentation, we should probably call it out as a bug fix in the next
> minor release notes, since users who have adopted the functions will
> likely want to update their versions.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp