Rethinking opclass member checks and dependency strength

On Wed, Aug 7, 2019 at 7:28 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Over in [1] we realized that it would be a good idea to remove the <@
operator from contrib/intarray's GiST opclasses. Unfortunately, doing
that isn't a simple matter of generating an extension update script
containing ALTER OPERATOR FAMILY DROP OPERATOR, because that operator
is marked as internally dependent on its opclass which means that
dependency.c will object. We could do some direct hacking on
pg_depend to let the DROP be allowed, but ugh.

I started to wonder why GiST opclass operators are ever considered
as required members of their opclass. The existing rule (cf.
opclasscmds.c) is that everything mentioned in CREATE OPERATOR CLASS
will have an internal dependency on the opclass, but if you add
operators or functions with ALTER OPERATOR FAMILY ADD, those just
have AUTO dependencies on their operator family. So the assumption
is that opclass creators will only put the bare minimum of required
stuff into CREATE OPERATOR CLASS and then add optional stuff with
ALTER ... ADD. But none of our contrib modules do it like that,
and I'd lay long odds against any third party code doing it either.

That's really odd. I don't think any extension SQL scripts does
really care about difference between operators defined in CREATE
OPERATOR CLASS and operators defined in ALTER OPERATOR FAMILY ADD.
But if they would care, then all GiST, GIN, SP-GiST and BRIN opclasses
would define all their operators using ALTER OPERATOR FAMILY ADD.

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.

If we went along this line, then a dump/restore or pg_upgrade
would be enough to change an opclass's dependencies to the new
style, which would get us to a place where intarray's problem
could be fixed with ALTER OPERATOR FAMILY DROP OPERATOR and
nothing else. Such an upgrade script wouldn't work in older
releases, but I think we don't generally care about that.

+1

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

But none of our contrib modules do it like that, and I'd lay long odds
against any third party code doing it either.

Thoughts?

PostGIS has some rarely used box operations as part of GiST opclass, like
"overabove".

These are source of misunderstanding, as it hinges on the fact that
non-square geometry will be coerced into a box on a call, which is not
obvious when you call it on something like diagonal linestrings.
It may happen that we will decide to remove them. On such circumstances, I
expect that ALTER OPERATOR CLASS DROP OPERATOR will work.

Other thing that I would expect is that DROP FUNCTION ... CASCADE will
remove the operator and unregister the operator from operator class without
dropping operator class itself.

Alexander Korotkov <a.korotkov@postgrespro.ru> writes:

On Wed, Aug 7, 2019 at 7:28 PM Tom Lane <tgl@sss.pgh.pa.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

On Sun, Aug 18, 2019 at 10:00 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

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.

Looks sufficient to me. I didn't yet imagine something else useful.

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

Flexibility also looks sufficient to me.

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

* 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 also not sure about this. Two lists look OK to me.

* 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

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Alexander Korotkov <a.korotkov@postgrespro.ru> writes:

On Sun, Aug 18, 2019 at 10:00 PM Tom Lane <tgl@sss.pgh.pa.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'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.

regards, tom lane

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.korotkov@postgrespro.ru> writes:

On Sun, Aug 18, 2019 at 10:00 PM Tom Lane <tgl@sss.pgh.pa.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

Tomas Vondra <tomas.vondra@2ndquadrant.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

On Sun, Jan 05, 2020 at 12:33:10PM -0500, Tom Lane wrote:

Tomas Vondra <tomas.vondra@2ndquadrant.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

Tomas Vondra <tomas.vondra@2ndquadrant.com> writes:

On Sun, Jan 05, 2020 at 12:33:10PM -0500, Tom Lane wrote:

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 :-(

Still haven't got a better naming idea, but in the meantime here's
a rebase to fix a conflict with 612a1ab76.

regards, tom lane

I wrote:

Still haven't got a better naming idea, but in the meantime here's
a rebase to fix a conflict with 612a1ab76.

I see from the cfbot that this needs another rebase, so here 'tis.
No changes in the patch itself.

regards, tom lane

On 2019-Aug-18, Tom Lane wrote:

* 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 created that file so that it'd be possible to interpret the struct
when dealing with DDL commands in event triggers (commit b488c580aef4).
The struct was previously in a .c file, and we didn't have an
appropriate .h file to put it in. I think amapi.h is a great place for
it.

--
�lvaro Herrera https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Alvaro Herrera <alvherre@2ndquadrant.com> writes:

On 2019-Aug-18, Tom Lane wrote:

* 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 created that file so that it'd be possible to interpret the struct
when dealing with DDL commands in event triggers (commit b488c580aef4).
The struct was previously in a .c file, and we didn't have an
appropriate .h file to put it in. I think amapi.h is a great place for
it.

Yeah, later versions of the patch put it there.

regards, tom lane

On 31.03.2020 23:45, Tom Lane wrote:

I wrote:

Still haven't got a better naming idea, but in the meantime here's
a rebase to fix a conflict with 612a1ab76.

Maybe "amadjustmembers" will work?

I've looked through the patch and noticed this comment:

+��� ��� ��� default:
+��� ��� ��� ��� /* Probably we should throw error here */
+��� ��� ��� ��� break;

I suggest adding an ERROR or maybe Assert, so that future developers
wouldn't
forget about setting dependencies. Other than that, the patch looks good
to me.

--
Anastasia Lubennikova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

The following review has been posted through the commitfest application:
make installcheck-world: tested, failed
Implements feature: tested, failed
Spec compliant: not tested
Documentation: not tested

I've gone through the patch and applied on the master branch, other than a few hunks, and whether as suggested upthread, the default case for "switch (op->number)" should throw an error or not, I found that bloom regression is crashing.
-------------
test bloom ... FAILED (test process exited with exit code 2) 20 ms

+server closed the connection unexpectedly
+	This probably means the server terminated abnormally
+	before or while processing the request.
+connection to server was lost
-------------

The new status of this patch is: Waiting on Author

Hamid Akhtar <hamid.akhtar@gmail.com> writes:

I've gone through the patch and applied on the master branch, other than a few hunks, and whether as suggested upthread, the default case for "switch (op->number)" should throw an error or not, I found that bloom regression is crashing.
-------------
test bloom ... FAILED (test process exited with exit code 2) 20 ms

Hmm ... I think you must have done something wrong. For me,
am-check-members-callback-5.patch still applies cleanly (just a few
small offsets), and it passes that test as well as the rest of
check-world. The cfbot agrees [1]https://travis-ci.org/github/postgresql-cfbot/postgresql/builds/712599990.

Maybe you didn't "make clean" before rebuilding?

regards, tom lane

[1]: https://travis-ci.org/github/postgresql-cfbot/postgresql/builds/712599990

On Tue, Jul 28, 2020 at 8:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Hamid Akhtar <hamid.akhtar@gmail.com> writes:

I've gone through the patch and applied on the master branch, other than

a few hunks, and whether as suggested upthread, the default case for
"switch (op->number)" should throw an error or not, I found that bloom
regression is crashing.

-------------
test bloom ... FAILED (test process exited with

exit code 2) 20 ms

Hmm ... I think you must have done something wrong. For me,
am-check-members-callback-5.patch still applies cleanly (just a few
small offsets), and it passes that test as well as the rest of
check-world. The cfbot agrees [1].

Maybe you didn't "make clean" before rebuilding?

regards, tom lane

[1]
https://travis-ci.org/github/postgresql-cfbot/postgresql/builds/712599990

I was pretty sure I did make clean before testing the patch, but perhaps I
didn't as re-running it causes all tests to pass.

Sorry for the false alarm. All good with the patch.

--
Highgo Software (Canada/China/Pakistan)
URL : www.highgo.ca
ADDR: 10318 WHALLEY BLVD, Surrey, BC
CELL：+923335449950 EMAIL: mailto:hamid.akhtar@highgo.ca
SKYPE: engineeredvirus

The following review has been posted through the commitfest application:
make installcheck-world: tested, passed
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested

Looks good to me.

CORRECTION:
In my previous review I had mistakenly mentioned that it was causing a server crash. Tests run perfectly fine without any errors.

The new status of this patch is: Ready for Committer

Anastasia Lubennikova <a.lubennikova@postgrespro.ru> writes:

On 31.03.2020 23:45, Tom Lane wrote:

Still haven't got a better naming idea, but in the meantime here's
a rebase to fix a conflict with 612a1ab76.

Maybe "amadjustmembers" will work?

Not having any better idea, I adopted that one.

I've looked through the patch and noticed this comment:
+                /* Probably we should throw error here */

I suggest adding an ERROR or maybe Assert, so that future developers
wouldn't
forget about setting dependencies. Other than that, the patch looks good
to me.

I'd figured that adding error checks could be left for a second pass,
but there's no strong reason not to insert these particular checks
now ... and indeed, doing so showed me that the patch hadn't been
updated to cover the recent addition of opclass options procs :-(.
So I fixed that and pushed it.

regards, tom lane