Hash index on macaddr -> crash

Started by Tom Laneabout 25 years ago6 messages
#1Tom Lane
tgl@sss.pgh.pa.us

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

#2Mikheev, Vadim
vmikheev@SECTORBASE.COM
In reply to: Tom Lane (#1)
RE: Hash index on macaddr -> crash

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

#3Mikheev, Vadim
vmikheev@SECTORBASE.COM
In reply to: Mikheev, Vadim (#2)
RE: Hash index on macaddr -> crash

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

#4Darren King
darrenk@insightdist.com
In reply to: Tom Lane (#1)
RE: Hash index on macaddr -> crash

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

#5Tom Lane
tgl@sss.pgh.pa.us
In reply to: Mikheev, Vadim (#3)
Re: Hash index on macaddr -> crash

"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

#6Tom Lane
tgl@sss.pgh.pa.us
In reply to: Darren King (#4)
Re: Hash index on macaddr -> crash

"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