Обсуждение: Hash index on macaddr -> crash

It was just pointed out on pggeneral that hash indexes on macaddr
columns don't work.  Looking into it, I find that someone (me :-()
made a booboo: pg_amproc claims that hashvarlena is the appropriate
hash function for macaddr --- but macaddr isn't a varlena type,
it's a fixed-length pass-by-reference type.

We could fix this either by adding a new hash function to support
macaddr, or by removing the pg_amXXX entries that claim macaddr is
hashable.  Either change will not take effect without an initdb,
however, and I'm loath to force one now that we've started beta.

What I'm inclined to do is add the hash function but not force an
initdb (ie, not increment catversion).  That would mean that people
running 7.1beta1 would still have the bug in 7.1 final if they don't
choose to do an initdb when they update.  But hashing macaddr isn't
very common (else we'd have noticed sooner!) so this seems OK, and
better than forcing an initdb on our beta testers.

Comments, objections?
        regards, tom lane

> We could fix this either by adding a new hash function to support
> macaddr, or by removing the pg_amXXX entries that claim macaddr is
> hashable.  Either change will not take effect without an initdb,
> however, and I'm loath to force one now that we've started beta.

If we're going to add CRC to log then we need in beta anyway...
Are we going?

Vadim

> > We could fix this either by adding a new hash function to support
> > macaddr, or by removing the pg_amXXX entries that claim macaddr is
> > hashable.  Either change will not take effect without an initdb,
> > however, and I'm loath to force one now that we've started beta.
> 
> If we're going to add CRC to log then we need 
> in beta anyway...
^^^^^^^^^
Ops - we need in INITDB...

> Are we going?

Vadim

> We could fix this either by adding a new hash function to support
> macaddr, or by removing the pg_amXXX entries that claim macaddr is
> hashable.  Either change will not take effect without an initdb,
> however, and I'm loath to force one now that we've started beta.

How about creating an SQL statement that will make the change and
putting a blurb about it it in the README, INSTALL and/or FAQ?

This wouldn't require an initdb and would let people have the fix.

For things like this that update exising fields (vs adding/deleting
fields hard-wired for use in the backend), it should work, no?

Darren

"Mikheev, Vadim" <vmikheev@SECTORBASE.COM> writes:
>>>> hashable.  Either change will not take effect without an initdb,
>>>> however, and I'm loath to force one now that we've started beta.
>> 
>> If we're going to add CRC to log then we need 
>> in beta anyway...
> Ops - we need in INITDB...

Not to mention adding a CRC to page headers, which was the other part of
the thread.

>> Are we going?

I dunno.  For now, I'll put in the hash function but not force an
initdb.  If we do the CRC thing then we'll have the initdb at that
point.
        regards, tom lane

"Darren King" <darrenk@insightdist.com> writes:
> How about creating an SQL statement that will make the change and
> putting a blurb about it it in the README, INSTALL and/or FAQ?

In theory we could do that, but I doubt it's worth the trouble.
Hash on macaddr has never worked (until my upcoming commit anyway ;-))
and the lack of complaints seems to be adequate evidence that no one
in the beta-test community has any use for it... so who's going to
go to the trouble of manually updating each of their databases?

My bet is that there'll be an initdb forced for some other reason
(like adding CRCs, or some much-more-serious bug than this one)
before 7.1 final anyway...
        regards, tom lane