Thread: ISN extension bug?

ISN extension bug?

From
Fabien COELHO
Date:
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.



Re: ISN extension bug? (with patch)

From
Fabien COELHO
Date:
> 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.

Re: ISN extension bug? (with patch)

From
Peter Eisentraut
Date:
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.



Re: ISN extension bug? (with patch)

From
Fabien COELHO
Date:
> 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.



Re: ISN extension bug? (with patch)

From
Peter Eisentraut
Date:
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.




Re: ISN extension bug? (with patch)

From
Fabien COELHO
Date:
>> 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

Re: ISN extension bug? (with patch)

From
Heikki Linnakangas
Date:
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



Re: ISN extension bug? (with patch)

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

Thanks!

-- 
Fabien.