Re: Rethinking opclass member checks and dependency strength

Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
> On Wed, Aug 7, 2019 at 7:28 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> This leads to the thought that maybe we could put some intelligence
>> into an index-AM-specific callback instead. For example, for btree
>> and hash the appropriate rule is probably that cross-type operators
>> and functions should be tied to the opfamily while single-type
>> members are internally tied to the associated opclass. For GiST,
>> GIN, and SPGiST it's not clear to me that *any* operator deserves
>> an INTERNAL dependency; only the implementation functions do.
>>
>> Furthermore, if we had an AM callback that were charged with
>> deciding the right dependency links for all the operators/functions,
>> we could also have it do some validity checking on those things,
>> thus moving some of the checks made by amvalidate into a more
>> useful place.

> +1, sounds like a plan for me.

Here's a preliminary patch along these lines. It adds an AM callback
that can adjust the dependency types before they're entered into
pg_depend. There's a lot of stuff that's open for debate and/or
remains to be done:

* Is the parameter list of amcheckmembers() sufficient, or should we
pass more info (if so, what)? In principle, the AM can always look
up anything else it needs to know from the provided OIDs, but that
would be cumbersome if many AMs need the same info.

* Do we need any more flexibility in the set of ways that the pg_depend
entries can be set up than what I've provided here?

* Are the specific ways that the entries are getting set up appropriate?
Note in particular that I left btree/hash alone, feeling that the default
(historical) behavior was designed for them and is not unreasonable; but
maybe we should switch them to the cross-type-vs-not-cross-type behavior
proposed above. Also I didn't touch BRIN because I don't know enough
about it to be sure what would be best, and I didn't touch contrib/bloom
because I don't care too much about it.

* I didn't add any actual error checking to the checkmembers functions.
I figure that can be done in a followup patch, and it'll just be tedious
boilerplate anyway.

* I refactored things a little bit in opclasscmds.c, mostly by adding
an is_func boolean to OpFamilyMember and getting rid of parameters
equivalent to that. This is based on the thought that AMs might prefer
to process the structs based on such a flag rather than by keeping them
in separate lists. We could go further and merge the operator and
function structs into one list, forcing the is_func flag to be used;
but I'm not sure whether that'd be an improvement.

* I'm not at all impressed with the name, location, or concept of
opfam_internal.h. I think we should get rid of that header and put
the OpFamilyMember struct somewhere else. Given that this patch
makes it part of the AM API, it wouldn't be unreasonable to move it
to amapi.h. But I've not done that here.

I'll add this to the upcoming commitfest.

regards, tom lane