Re: ISN extension bug? (with patch)

Hello devs,

ISTM that there is an issue on the ISMN type:

sh> psql
psql (9.3.2)
Type "help" for help.
# CREATE EXTENTION isn;
# SELECT ISMN 'M123456782';
M-1234-5678-5

*** The 2 is changed to 5 in the display...

# SELECT ISMN 'M123456785';
ERROR: invalid check digit for ISMN number: "M123456785", should be 2
LINE 1: SELECT ISMN 'M123456785';
^
# SELECT ISMN 'M-1234-5678-5';
ERROR: invalid check digit for ISMN number: "M-1234-5678-5", should be 2
LINE 1: SELECT ISMN 'M-1234-5678-5';
^

--
Fabien.

> sh> psql
> psql (9.3.2)
> Type "help" for help.
> # CREATE EXTENTION isn;
> # SELECT ISMN 'M123456782';
> M-1234-5678-5
> # SELECT ISMN 'M123456785';
> ERROR: invalid check digit for ISMN number: "M123456785", should be 2
> LINE 1: SELECT ISMN 'M123456785';

With the attached one liner patch compiled with pgxs:

# SELECT ISMN 'M123456785';
M-1234-5678-5

I'm not sure whether the policy is to update the version number of the
extension for such a change. As the library is always "isn.so", two
versions cannot live in parallel anyway. If it is useful, the second patch
attached also upgrade the version number.

Also, I notice that there is no test for this module, so I do not know
whether I've broken anything, or whether there are other simular bugs.
ISTM that it is not the case because I could test other types.

Because of the complexity of the code (there are embedded automata with
plenty of gotos and pointer arithmetic), I find this astoundingly unwise.
If the code was cleaner/simpler, it would just find it very unwise:-) Thus
I would suggest to add to some todo list: "check that all extensions have
regression tests, and add some if not."

--
Fabien.

On 12/22/13, 2:36 AM, Fabien COELHO wrote:
> I'm not sure whether the policy is to update the version number of the
> extension for such a change. As the library is always "isn.so", two
> versions cannot live in parallel anyway. If it is useful, the second
> patch attached also upgrade the version number.

If you are not changing anything in the SQL, then you don't need to
change the version number.

> On 12/22/13, 2:36 AM, Fabien COELHO wrote:
>> I'm not sure whether the policy is to update the version number of the
>> extension for such a change. As the library is always "isn.so", two
>> versions cannot live in parallel anyway. If it is useful, the second
>> patch attached also upgrade the version number.
>
> If you are not changing anything in the SQL, then you don't need to
> change the version number.

Ok, thanks for the information. I understand that the version number is
about the API, not the implementation.

If so, there is only the one-liner patch to consider.

--
Fabien.

On 12/24/13, 10:29 AM, Fabien COELHO wrote:
>
>> On 12/22/13, 2:36 AM, Fabien COELHO wrote:
>>> I'm not sure whether the policy is to update the version number of the
>>> extension for such a change. As the library is always "isn.so", two
>>> versions cannot live in parallel anyway. If it is useful, the second
>>> patch attached also upgrade the version number.
>>
>> If you are not changing anything in the SQL, then you don't need to
>> change the version number.
>
> Ok, thanks for the information. I understand that the version number is
> about the API, not the implementation.
>
> If so, there is only the one-liner patch to consider.

This patch doesn't apply anymore. Please submit an updated patch for
the commit fest.

>> If so, there is only the one-liner patch to consider.
>
> This patch doesn't apply anymore. Please submit an updated patch for
> the commit fest.

In src/include/utils/elog.h there is an include for "utils/errcodes.h"
which is generated somehow when compiling postgresql but not present by
default. So you have to compile postgresql and then the contrib, or use
PGXS with an already installed version.

With this caveat, the one-liner patch (4 characters removed) reattached
does compile for me:

sh> git branch ismn2
sh> git checkout ismn2
sh> patch -p1 < ~/ismn-checksum.patch
patching file contrib/isn/isn.c
sh> ...
sh> cd contrib/isn
sh> make
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -fpic -I. -I. -I../../src/include -D_GNU_SOURCE -c -o isn.o isn.c
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -fpic -L../../src/port -L../../src/common -Wl,--as-needed -Wl,-rpath,'/usr/local/pgsql/lib',--enable-new-dtags -shared -o isn.so isn.o
sh>

--
Fabien

On 01/03/2014 07:53 PM, Fabien COELHO wrote:
>
>>> If so, there is only the one-liner patch to consider.
>>
>> This patch doesn't apply anymore. Please submit an updated patch for
>> the commit fest.
>
> In src/include/utils/elog.h there is an include for "utils/errcodes.h"
> which is generated somehow when compiling postgresql but not present by
> default. So you have to compile postgresql and then the contrib, or use
> PGXS with an already installed version.
>
> With this caveat, the one-liner patch (4 characters removed) reattached
> does compile for me:

Thanks, applied.

- Heikki

>> With this caveat, the one-liner patch (4 characters removed) reattached
>> does compile for me:
>
> Thanks, applied.

Thanks!

--
Fabien.