Re: ISN patch that applies cleanly with git apply

On Mon, Oct 18, 2010 at 11:17 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Peter Geoghegan <peter.geoghegan86@gmail.com> writes:
>>> I may be in the minority here, but I'm inclined to just apply this and
>>> move on.  I agree with your concerns that this whole module is badly
>>> designed and that the approach of hard-coding the list of ISBN ranges
>>> is fundamentally unscalable, but unless we're going to rip it out or
>>> rearchitect it, we don't really get any benefit out of digging in our
>>> heels and refusing to apply occasional band-aids.
>
>> I don't think you'll be in the minority. I suspect that we'll end up
>> applying this patch, because your reasoning is sound.
>
> Personally I was hoping for some independent validation that the
> proposed changes match current reality in ISN-land.  But apparently
> no one actually wants to repeat the research, so we might as well just
> take the changes on faith.
>
>>> 2. Don't try to validate the list of ISBN ranges at all.
>
>> Actually, we just use the range information to hyphenate ISBNs...they
>> won't actually be rejected unless they have an invalid check digit,
>> or, in the case of ISBN-13s, if their country codes (first 3 digits)
>> aren't "bookland", either 978 or 979. Perhaps the problem of new
>> ranges being introduced over time isn't all that much of a problem.
>
> Yeah, in the real world this is not all that important.
>
>> Could this patch be backported as a bug fix?
>
> This isn't a bug fix in my opinion.  It's a behavioral change, and one
> with nonzero risk of breaking apps that might not expect the new
> hyphenation.
>
>> It adds the new bookland
>> country code of 979. Prior versions of the patch will outright reject
>> these correct ISBN-13s, so I think that is a good idea.
>
> Hm, maybe just the addition of 979 is ok to back-port.  Comments?

I vote for just applying it to HEAD.  People can use the new version
of just this module with old versions of PG if they want the new
behavior.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company