Re: Rethinking opclass member checks and dependency strength

On Sun, Jan 05, 2020 at 12:33:10PM -0500, Tom Lane wrote:
>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.
>

Interesting. I still get

$ git am ~/am-check-members-callback-3.patch
Patch format detection failed.

I'm on git 2.21.1, not sure if that matters. Cputube is happy, though.

Meh.

>> 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.
>

OK. So we shall keep the v2 behavior, with opfamily dependencies and
modified pg_dump output? Fine with me - I still think it's a bit weird,
but I'm willing to commit myself to add the missing syntax. And I doubt
anyone will notice, probably ...

>> 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.
>

OK.

>
>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?
>

I don't :-(

regards

--
Tomas Vondra http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services