Re: Rethinking opclass member checks and dependency strength

Hi,

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.").
In any case, this means cputube can't apply/test it.

On Sat, Sep 14, 2019 at 07:01:33PM -0400, Tom Lane wrote:
>Alexander Korotkov <a(dot)korotkov(at)postgrespro(dot)ru> writes:
>> On Sun, Aug 18, 2019 at 10:00 PM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>>> * 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 think we need ability to remove GiST fetch proc. Presence of this
>> procedure is used to determine whether GiST index supports index only
>> scan (IOS). We need to be able to remove this proc to drop IOS
>> support.
>
>OK ... so thinking in more general terms, you're arguing that any optional
>support function should have a soft not hard dependency. The attached v2
>patch implements that rule, and also changes btree and hash to use
>the cross-type-vs-not-cross-type behavior I proposed earlier.
>
>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'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.
>
>> +1
>
>Did that in this revision, too.
>

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.

regards

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