Thread: ISN extension bug?
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.