Re: [PATCH] cleanup hashindex for pg_migrator hashindex compat mode (for 8.4)

Tom Lane píše v ne 24. 05. 2009 v 18:46 -0400:
> Kenneth Marshall <ktm(at)rice(dot)edu> writes:
> > On Sat, May 23, 2009 at 02:52:49PM -0400, Zdenek Kotala wrote:
> >>> Attached patch cleanups hash index headers to allow compile hasham for
> >>> 8.3 version. It helps to improve pg_migrator with capability to migrate
> >>> database with hash index without reindexing.
>
> > How does that work with the updated hash functions without a reindex?
>
> I looked at this patch and I don't see how it helps pg_migrator at all.
> It's just pushing some code and function declarations around.

The main important thing is move hash_any/hash_uint32 function from hash
am. It should reduce amount of duplicated code necessary for old hash
index implementation. Rest is only rearrangement and cleanup.

> The rearrangement might be marginally nicer from a code beautification
> point of view --- right now we're a bit inconsistent about whether
> datatype-specific hash functions live in hashfunc.c or in the datatype's
> utils/adt/ file.

I personally prefer to keep it in type definition. AM should be
independent on types which are installed and data type code should be
self contained.

> But I'm not sure that removing hashfunc.c altogether is
> an appropriatera solution to that, not least because of the loss of CVS
> history for the functions. I'd be inclined to leave the core hash_any()
> code where it is, if not all of these functions altogether.

Until we will have better version control system, hashfunc.c can stay
here, but what I need is hashfunc.h. Minimalistic version of this patch
is to create hashfuct.h and modified related #include in C and header
files.

> What does seem useful is to refactor the headers so that datatype hash
> functions don't need to include all of the AM's implementation details.
> But this patch seems to do both more and less than that --- I don't
> think it's gotten rid of all external #includes of access/hash.h, and
> in any case moving the function code is not necessary to that goal.

Agree, I will prepare minimalistic version with hashfunc.h only.

> In any case, the barriers to implementing 8.3-style hash indexes in 8.4
> are pretty huge: you'd need to duplicate not only the hash AM code, but
> also all the hash functions, and therefore all of the hash pg_amop and
> pg_amproc entries.

I'm not sure if I need duplicate functions. Generally yes but It seems
to me that hash index does not changed functions behavior and they could
be shared at this moment.

> Given the close-to-zero usefulness of hash indexes
> in production installations, I don't think it's worth the trouble. It
> would be much more helpful to look into supporting 8.3-compatible GIN
> indexes.

Agree, I wanted to quickly verify function naming collision problem and
HASH index seems to me better candidate for this basic test. GIN has
priority.

Zdenek