Re: Rethinking opclass member checks and dependency strength

Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com> writes:
> The latest version of this patch (from 2019/09/14) no longer applies,
> although maybe it's some issue with patch format (applying it using
> patch works fine, git am fails with "Patch format detection failed.").

Hm, seems to be just a trivial conflict against the copyright-date-update
patch. Updated version attached.

> On Sat, Sep 14, 2019 at 07:01:33PM -0400, Tom Lane wrote:
>> This change results in a possibly surprising change in the expected output
>> for the 002_pg_dump.pl test: an optional support function that had been
>> created as part of CREATE OPERATOR CLASS will now be dumped as part of
>> ALTER OPERATOR FAMILY. Maybe that's too surprising? Another approach
>> that we could use is to give up the premise that soft dependencies are
>> always on the opfamily. If we kept the dependencies pointing to the
>> same objects as before (opclass or opfamily) and only twiddled the
>> dependency strength, then pg_dump's output would not change. Now,
>> this would possibly result in dropping a still-useful family member
>> if it were incorrectly tied to an opclass that's dropped --- but that
>> would have happened before, too. I'm not quite sure if we really want
>> to editorialize on the user's decisions about which grouping to tie
>> family members to.

> I agree it's a bit weird to add a dependency on an opfamily and not the
> opclass. Not just because of the pg_dump weirdness, but doesn't it mean
> that after a DROP OPERATOR CLASS we might still reject a DROP OPERATOR
> because of the remaining opfamily dependency? (Haven't tried, so maybe
> that works fine).

I poked at the idea of retaining the user's decisions as to whether
a member object is a member of an individual opclass or an opfamily,
but soon realized that there's a big problem with that: we don't have
any ALTER OPERATOR CLASS ADD/DROP syntax, only ALTER OPERATOR FAMILY.
So there's no way to express the concept of "add this at the opclass
level", if you forgot to add it during initial opclass creation.

I suppose that some case could be made for adding such syntax, but
it seems like a significant amount of work, and in the end it seems
like it's better to trust the system to get these assignments right.
Letting the user do it doesn't add much except the opportunity
to shoot oneself in the foot.

> One minor comment from me is that maybe "amcheckmembers" is a bit
> misleading. In my mind "check" implies a plain passive check, not
> something that may actually tweak the dependency type.

Hmm. I'm not wedded to that name, but do you have a better proposal?
The end goal (not realized in this patch, of course) is that these
callbacks would perform fairly thorough checking of opclass members,
missing only the ability to check that all required members are present.
So I don't want to name them something like "amfixdependencies", even
if that's all they're doing right now.

I see your point that "check" suggests a read-only operation, but
I'm not sure about a better verb. I thought of "amvalidatemembers",
but that's not really much better than "check" is it?

regards, tom lane