Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Tom Lane wrote:
>>> ... btw, what is the point of catalog/opfam_internal.h?
> The whole point of splitting the struct declaration to a new header was
> to get a DDL deparser to examine the list of objects being created, so
> that it could construct the deparsed command. If the struct is opaque
> to the outside world, there's no way to do that (as I recall, you can't
> construct the full command starting from the parsed version only -- you
> need access to the OIDs of the ops/procs actually added to the opclass.)
Hm. OK, looking closer, I see that we've effectively exposed these
structs as being what is passed to EventTriggerCollectCreateOpClass, so
even if there's not currently any committed code that can read that info,
we clearly need the structs to be accessible to interested event triggers.
I'm not sold that that was a great design, but it's what's there.
>> Regardless of that, I'm a bit skeptical that any of these structs ought
>> to be defined as part of the amapi.h interface. They seem to be making
>> premature judgments as to what an opclass verifier will care about. In
>> any case, tying the opclass verifier API to DDL-command implementation
>> details doesn't seem like a good plan.
> That's a different argument, with which I don't necessarily disagree.
> I think it's not unlikely that a verifier will want to examine the
> contents of the struct, but I don't think that means we need to expose
> the struct in amapi.h.
I think we'd be considerably better off to *not* re-use OpFamilyMember
in the verifier API. It's a struct that was only ever designed for
internal use in CREATE/ALTER OPERATOR CLASS.
I'm kind of inclined to just let the verifiers read the catalogs for
themselves. AFAICS, a loop around the results of SearchSysCacheList
is not going to be significantly more code than what this patch does,
and it avoids presuming that we know what the verifiers will wish to
look at.
regards, tom lane