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

From: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
To: Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)jp>
Cc: nagata(at)sraoss(dot)co(dot)jp, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Fix a Oracle-compatible instr function in the documentation
Date: 2018-01-07 03:00:09
Message-ID: 14698.1515294009@sss.pgh.pa.us
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Tatsuo Ishii <ishii(at)sraoss(dot)co(dot)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

Attachment Content-Type Size
fix-instr-doc-2.patch text/x-diff 4.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Ryan Murphy 2018-01-07 03:57:10 Re: Add default role 'pg_access_server_files'
Previous Message Tom Lane 2018-01-07 00:58:37 Re: proposal: alternative psql commands quit and exit