Re: BUG #13442: ISBN doesn't always roundtrip with text

On 06/20/2015 10:04 AM, Fabien COELHO wrote:
>
>>> I think this definitely indicates a bug:
>>>
>>> regression=# select '9791020902573'::isbn;
>>>      isbn
>>> ---------------
>>> 10-209-0257-4
>>> (1 row)
>>
>> Yes, indeed, this one is definitely a bug.
>
> Attached patch makes isn to distinguish when needed between ISBN and
> ISBN13. ISBN is the legacy ISBN10 number which is part of ISBN13, but not
> all ISBN13 numbers are ISBN.
>
> The initial version was confusing both types, leading to accept valid
> ISBN13 number as valid ISBN(10) number although they use the 979 prefix,
> hence the reported issues.
>
> The patch also adds some tests.
>
> There is no clear upgrade path from 1.0 to 2.0, as the ISBN "in" function
> is changed, as well as some casts.  I'm not sure of a safe way to upgrade,
> because the initial version was accepting invalid ISBN which would now be
> rejected, thus some store data may be considered corrupted in a database.
> On the other hand, probably few people would use a legacy ISBN type and
> put ISBN13 number in them, hopefully? Hmmm...

10-digit ISBNs are indeed legacy, I don't have much sympathy for an
application that still uses the 10-digit isbn datatype in a table. The
cast might well be used for display purposes, though. You might do
something like "SELECT isbn_column as bisbn13, isbn_column::isbn as isbn
FROM ...". So I think your patch is OK, and we should provide an upgrade
script, and put a warning in the release notes that you should not be
using the 10-digit isbn datatype as a column type anymore.

As long as you don't use the isbn datatype in any tables or views, the
upgrade script could just DROP the datatype, and recreate it. It's
unfortunate that it won't work if you've used it in a view, but it seems
acceptable. Could you write an upgrade script like that, please?

In the long term, I think we should deprecate all the legacy "short"
datatypes, isbn, issn, ismn and upc. As a replacement for display
purposes, provide functions to print an ean13 in those legacy forms. I'm
not even sure we need the isbn13, issn13 etc. datatypes, to be honest. A
single ean13 datatype and a function to check if an EAN is an ISBN, ISSN
etc. should be enough. You could then use that in a CHECK constraint or
create a domain if you really need to. Oh, and get rid of the "weak
input mode" while we're at it - that's just horrible.

Perhaps we should add those functions now, so that you can rewrite your
application to use them now, and we could remove the legacy datatypes in
a future release.

- Heikki