Re: knngist patch support

Robert Haas <robertmhaas@gmail.com> writes:
> On Sat, Feb 13, 2010 at 2:03 PM, Joshua Tolley <eggyknap@gmail.com> wrote:
>> (Realizing I'm a lurker in this conversation, and hoping not to ask irritating
>> questions) Do we need to rename SearchSysCache et al. to SearchSysCache1,
>> etc.? It seems to me that requires changes to all kinds of software without
>> any real need. The four lines of PL/LOLCODE that inspired this thought aren't
>> themselves a great burden, but when combined with everyone else using
>> SearchSysCache already...

> If we want to allow 5-key syscaches, we have to add an extra parameter
> to SearchSysCache and friends.  So everyone caller of SearchSysCache
> is going to break.  (Well, unless we instead leave SearchSysCache
> alone and add SearchSysCacheExtended or similar; but that's not really
> project style.)

It's fair to stop and think about how this would affect external
packages and what they'd have to do to support building against both new
and old-style backends.

Reflecting on it, it seems to me that the separate SearchSysCacheN()
macros are obviously cleaner and closer to preferred project style than
the existing code with all those explicit zeroes.  So I think there's
a case for migrating to that style even if we didn't have a concern
about the max number of lookup keys.

What would probably be the recommended solution for backwards-compatible
source code is to convert the actual calls to new style, and then
provide a block of macro definitions along the lines of

#if CATALOG_VERSION_NO < something
#define SearchSysCache1(...) SearchSysCache(..., 0, 0, 0)
#define SearchSysCache2(...) SearchSysCache(..., 0, 0)
etc

So that seems okay so far.

What's not clear from this is whether we should reserve SearchSysCache
as an equivalent to SearchSysCache4() and therefore name the underlying
function something different.  That would allow being lazy about
converting individual calls over.

I'm inclined to think not, and here's the reason: with the old call
style it was never apparent from the callee side how many arguments the
caller intended to be valid, and so for example we couldn't have an
Assert that checked it was right.  I'm not sure if we should go so
far as to add an explicit nkeys argument to SearchSysCache, but it's
worth thinking about at least.
        regards, tom lane