Re: Rethinking opclass member checks and dependency strength
Anastasia Lubennikova 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
Re: Rethinking opclass member checks and dependency strength
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
Re: Rethinking opclass member checks and dependency strength
On Tue, Jul 28, 2020 at 8:43 PM Tom Lane wrote: > Hamid Akhtar 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/71250 > 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.akh...@highgo.ca SKYPE: engineeredvirus
Re: Rethinking opclass member checks and dependency strength
Hamid Akhtar 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/71250
Re: Rethinking opclass member checks and dependency strength
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
Re: Rethinking opclass member checks and dependency strength
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
Re: Rethinking opclass member checks and dependency strength
Alvaro Herrera 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
Re: Rethinking opclass member checks and dependency strength
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 Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: Rethinking opclass member checks and dependency strength
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
diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c
index d3bf866..663ff7e 100644
--- a/contrib/bloom/blutils.c
+++ b/contrib/bloom/blutils.c
@@ -139,6 +139,7 @@ blhandler(PG_FUNCTION_ARGS)
amroutine->amproperty = NULL;
amroutine->ambuildphasename = NULL;
amroutine->amvalidate = blvalidate;
+ amroutine->amcheckmembers = NULL;
amroutine->ambeginscan = blbeginscan;
amroutine->amrescan = blrescan;
amroutine->amgettuple = NULL;
diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml
index 37f8d87..d76d17d 100644
--- a/doc/src/sgml/indexam.sgml
+++ b/doc/src/sgml/indexam.sgml
@@ -141,6 +141,7 @@ typedef struct IndexAmRoutine
amproperty_function amproperty; /* can be NULL */
ambuildphasename_function ambuildphasename; /* can be NULL */
amvalidate_function amvalidate;
+amcheckmembers_function amcheckmembers; /* can be NULL */
ambeginscan_function ambeginscan;
amrescan_function amrescan;
amgettuple_function amgettuple; /* can be NULL */
@@ -500,7 +501,48 @@ amvalidate (Oid opclassoid);
the access method can reasonably do that. For example, this might include
testing that all required support functions are provided.
The amvalidate function must return false if the opclass is
- invalid. Problems should be reported with ereport messages.
+ invalid. Problems should be reported with ereport
+ messages, typically at INFO level.
+
+
+
+
+void
+amcheckmembers (Oid opfamilyoid,
+Oid opclassoid,
+List *operators,
+List *functions);
+
+ Validate proposed operator and function members of an operator family,
+ so far as the access method can reasonably do that, and set their
+ dependency types if the default is not satisfactory. This is called
+ during CREATE OPERATOR CLASS and during
+ ALTER OPERATOR FAMILY ADD; in the latter
+ case opclassoid is InvalidOid.
+ The List arguments are lists
+ of OpFamilyMember structs, as defined
+ in amapi.h.
+
+ Tests done by this function will typically be a subset of those
+ performed by amvalidate,
+ since amcheckmembers cannot assume that it is
+ seeing a complete set of members. For example, it would be reasonable
+ to check the signature of a support function, but not to check whether
+ all required support functions are provided. Any problems can be
+ reported by throwing an error.
+
+ The dependency-related fields of
+ the OpFamilyMember structs are initialized by
+ the core code to create hard dependencies on the opclass if this
+ is CREATE OPERATOR CLASS, or soft dependencies on the
+ opfamily if this is ALTER OPERATOR FAMILY ADD.
+ amcheckmembers can adjust these fields if some other
+ behavior is more appropriate. For example, GIN, GiST, and SP-GiST
+ always set operator members to have soft dependencies on the opfamily,
+ since the connection between an operator and an opclass is relatively
+ weak in these index types; so it is reasonable to allow operator members
+ to be added and removed freely. Optional support functions are typically
+ also given soft dependencies, so that they can be removed if necessary.
diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c
index 7db3ae5..b911029 100644
--- a/src/backend/access/brin/brin.c
+++ b/src/backend/access/brin/brin.c
@@ -120,6 +120,7 @@ brinhandler(PG_FUNCTION_ARGS)
amroutine->amproperty = NULL;
amroutine->ambuildphasename = NULL;
amroutine->amvalidate = brinvalidate;
+ amroutine->amcheckmembers = NULL;
amroutine->ambeginscan = brinbeginscan;
amroutine->amrescan = brinrescan;
amroutine->amgettuple = NULL;
diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c
index a400f1f..26aa152 100644
--- a/src/backend/access/gin/ginutil.c
+++ b/src/backend/access/gin/ginutil.c
@@ -71,6 +71,7 @@ ginhandler(PG_FUNCTION_ARGS)
amroutine->amproperty = NULL;
amroutine->ambuildphasename = NULL;
amroutine->amvalidate = ginvalidate;
+ amroutine->amcheckmembers = gincheckmembers;
amroutine->ambeginscan = ginbeginscan;
amroutine->amrescan = ginrescan;
amroutine->amgettuple = NULL;
diff --git a/src/backend/access/gin/ginvalidate.c b/src/backend/access/gin/ginvalidate.c
index 1e3046f..739006a 100644
--- a/src/backend/access/gin/ginvalidate.c
+++ b/src/backend/access/gin/ginvalidate.c
@@ -271,3 +271,64 @@ ginvalidate(Oid opclassoid)
return result;
}
+
+/*
+ * Prechecking function for adding operators/functions to a GIN opfamily.
+ */
+void
+gincheckmembers(Oid opfamilyoid,
+Oid opclassoid,
+List *operators,
+List *functions)
+{
+ ListCell *lc;
+
+ /*
+ * Operator members of a GIN
Re: Rethinking opclass member checks and dependency strength
Tomas Vondra 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 diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c index 0104d02..1098ab7 100644 --- a/contrib/bloom/blutils.c +++ b/contrib/bloom/blutils.c @@ -138,6 +138,7 @@ blhandler(PG_FUNCTION_ARGS) amroutine->amproperty = NULL; amroutine->ambuildphasename = NULL; amroutine->amvalidate = blvalidate; + amroutine->amcheckmembers = NULL; amroutine->ambeginscan = blbeginscan; amroutine->amrescan = blrescan; amroutine->amgettuple = NULL; diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml index 37f8d87..d76d17d 100644 --- a/doc/src/sgml/indexam.sgml +++ b/doc/src/sgml/indexam.sgml @@ -141,6 +141,7 @@ typedef struct IndexAmRoutine amproperty_function amproperty; /* can be NULL */ ambuildphasename_function ambuildphasename; /* can be NULL */ amvalidate_function amvalidate; +amcheckmembers_function amcheckmembers; /* can be NULL */ ambeginscan_function ambeginscan; amrescan_function amrescan; amgettuple_function amgettuple; /* can be NULL */ @@ -500,7 +501,48 @@ amvalidate (Oid opclassoid); the access method can reasonably do that. For example, this might include testing that all required support functions are provided. The amvalidate function must return false if the opclass is - invalid. Problems should be reported with ereport messages. + invalid. Problems should be reported with ereport + messages, typically at INFO level. + + + + +void +amcheckmembers (Oid opfamilyoid, +Oid opclassoid, +List *operators, +List *functions); + + Validate proposed operator and function members of an operator family, + so far as the access method can reasonably do that, and set their + dependency types if the default is not satisfactory. This is called + during CREATE OPERATOR CLASS and during + ALTER OPERATOR FAMILY ADD; in the latter + case opclassoid is InvalidOid. + The List arguments are lists + of OpFamilyMember structs, as defined + in amapi.h. + + Tests done by this function will typically be a subset of those + performed by amvalidate, + since amcheckmembers cannot assume that it is + seeing a complete set of members. For example, it would be reasonable + to check the signature of a support function, but not to check whether + all required support functions are provided. Any problems can be + reported by throwing an error. + + The dependency-related fields of + the OpFamilyMember structs are initialized by + the core code to create hard dependencies on the opclass if this + is CREATE OPERATOR CLASS, or soft dependencies on the + opfamily if this is ALTER OPERATOR FAMILY ADD. + amcheckmembers can adjust these fields if some other + behavior is more appropriate. For example, GIN, GiST, and SP-GiST + always set operator members to have soft dependencies on the opfamily, + since the connection between an operator and an opclass is relatively + weak in these index types; so it is reasonable to allow operator members + to be added and removed freely. Optional support functions are typically + also given soft dependencies, so that they can be removed if necessary. diff --git a/src/backend/access/brin/brin.c b/src/backend/access/brin/brin.c index 2e8f67e..d1e1176 100644 --- a/src/backend/access/brin/brin.c +++ b/src/backend/access/brin/brin.c @@ -118,6 +118,7 @@ brinhandler(PG_FUNCTION_ARGS) amroutine->amproperty = NULL; amroutine->ambuildphasename = NULL; amroutine->amvalidate = brinvalidate; + amroutine->amcheckmembers = NULL; amroutine->ambeginscan = brinbeginscan; amroutine->amrescan = brinrescan; amroutine->amgettuple = NULL; diff --git a/src/backend/access/gin/ginutil.c b/src/backend/access/gin/ginutil.c index a7e55ca..e61e629 100644 --- a/src/backend/access/gin/ginutil.c +++ b/src/backend/access/gin/ginutil.c @@ -70,6 +70,7 @@ ginhandler(PG_FUNCTION_ARGS) amroutine->amproperty = NULL; amroutine->ambuildphasename = NULL; amroutine->amvalidate = ginvalidate; + amroutine->amcheckmembers = gincheckmembers; amroutine->ambeginscan = ginbeginscan; amroutine->amrescan = ginrescan; amroutine->amgettuple = NULL; diff --git a/src/backend/access/gin/ginvalidate.c b/src/backend/access/gin/ginvalidate.c index 0b62e0a..f44aa4d 100644 --- a/src/backend/access/gin/ginvalidate.c +++ b/src/backend/access/gin/ginvalidate.c @@ -267,3 +267,64 @@ ginvalidate(Oid opclassoid) return result; } + +/* + * Prechecking function for adding operators/functions to a GIN
Re: Rethinking opclass member checks and dependency strength
On Sun, Jan 05, 2020 at 12:33:10PM -0500, Tom Lane wrote: Tomas Vondra 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
Re: Rethinking opclass member checks and dependency strength
Tomas Vondra 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 diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c index 23d959b..b016be1 100644 --- a/contrib/bloom/blutils.c +++ b/contrib/bloom/blutils.c @@ -134,6 +134,7 @@ blhandler(PG_FUNCTION_ARGS) amroutine->amproperty = NULL; amroutine->ambuildphasename = NULL; amroutine->amvalidate = blvalidate; + amroutine->amcheckmembers = NULL; amroutine->ambeginscan = blbeginscan; amroutine->amrescan = blrescan; amroutine->amgettuple = NULL; diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml index dd54c68..2d16c6f 100644 --- a/doc/src/sgml/indexam.sgml +++ b/doc/src/sgml/indexam.sgml @@ -137,6 +137,7 @@ typedef struct IndexAmRoutine amproperty_function amproperty; /* can be NULL */ ambuildphasename_function ambuildphasename; /* can be NULL */ amvalidate_function amvalidate; +amcheckmembers_function amcheckmembers; /* can be NULL */ ambeginscan_function ambeginscan; amrescan_function amrescan; amgettuple_function amgettuple; /* can be NULL */ @@ -496,7 +497,48 @@ amvalidate (Oid opclassoid); the access method can reasonably do that. For example, this might include testing that all required support functions are provided. The amvalidate function must return false if the opclass is - invalid. Problems should be reported with ereport messages. + invalid. Problems should be reported with ereport + messages, typically at INFO level. + + + + +void +amcheckmembers (Oid opfamilyoid, +Oid opclassoid, +List *operators, +List *functions); + + Validate proposed operator and function members of an operator family, + so far as the access method can reasonably do that, and set their + dependency types if the default is not satisfactory. This is called + during CREATE OPERATOR CLASS and
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 writes: On Sun, Aug 18, 2019 at 10:00 PM Tom Lane 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
Re: Rethinking opclass member checks and dependency strength
Alexander Korotkov writes: > On Sun, Aug 18, 2019 at 10:00 PM Tom Lane 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 diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c index cc16709..5f664ca 100644 --- a/contrib/bloom/blutils.c +++ b/contrib/bloom/blutils.c @@ -134,6 +134,7 @@ blhandler(PG_FUNCTION_ARGS) amroutine->amproperty = NULL; amroutine->ambuildphasename = NULL; amroutine->amvalidate = blvalidate; + amroutine->amcheckmembers = NULL; amroutine->ambeginscan = blbeginscan; amroutine->amrescan = blrescan; amroutine->amgettuple = NULL; diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml index dd54c68..2d16c6f 100644 --- a/doc/src/sgml/indexam.sgml +++ b/doc/src/sgml/indexam.sgml @@ -137,6 +137,7 @@ typedef struct IndexAmRoutine amproperty_function amproperty; /* can be NULL */ ambuildphasename_function ambuildphasename; /* can be NULL */ amvalidate_function amvalidate; +amcheckmembers_function amcheckmembers; /* can be NULL */ ambeginscan_function ambeginscan; amrescan_function amrescan; amgettuple_function amgettuple; /* can be NULL */ @@ -496,7 +497,48 @@ amvalidate (Oid opclassoid); the access method can reasonably do that. For example, this might include testing that all required support functions are provided. The amvalidate function must return false if the opclass is - invalid. Problems should be reported with ereport messages. + invalid. Problems should be reported with ereport + messages, typically at INFO level. + + + + +void +amcheckmembers (Oid opfamilyoid, +Oid opclassoid, +List *operators, +List *functions); + + Validate proposed operator and function members of an operator family, + so far as the access method can reasonably do that, and set their + dependency types if the default is not satisfactory. This is called + during CREATE OPERATOR CLASS and during + ALTER OPERATOR FAMILY ADD; in the latter + case opclassoid is InvalidOid. + The List arguments are lists + of OpFamilyMember structs, as defined + in amapi.h. + + Tests done by this function will typically be a subset of those + performed by amvalidate, + since amcheckmembers cannot assume that it is + seeing a complete set of members. For example, it would be reasonable + to check the signature of a support function, but not to check whether + all required support functions are provided. Any problems can be + reported by throwing an error. + + The dependency-related fields of + the OpFamilyMember structs are initialized by + the core code to create hard dependencies on the opclass if this + is CREATE OPERATOR CLASS, or soft dependencies on the +
Re: Rethinking opclass member checks and dependency strength
On Sun, Aug 18, 2019 at 10:00 PM Tom Lane 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
Re: Rethinking opclass member checks and dependency strength
Alexander Korotkov writes: > On Wed, Aug 7, 2019 at 7:28 PM Tom Lane 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 diff --git a/contrib/bloom/blutils.c b/contrib/bloom/blutils.c index cc16709..5f664ca 100644 --- a/contrib/bloom/blutils.c +++ b/contrib/bloom/blutils.c @@ -134,6 +134,7 @@ blhandler(PG_FUNCTION_ARGS) amroutine->amproperty = NULL; amroutine->ambuildphasename = NULL; amroutine->amvalidate = blvalidate; + amroutine->amcheckmembers = NULL; amroutine->ambeginscan = blbeginscan; amroutine->amrescan = blrescan; amroutine->amgettuple = NULL; diff --git a/doc/src/sgml/indexam.sgml b/doc/src/sgml/indexam.sgml index dd54c68..2815098 100644 --- a/doc/src/sgml/indexam.sgml +++ b/doc/src/sgml/indexam.sgml @@ -137,6 +137,7 @@ typedef struct IndexAmRoutine amproperty_function amproperty; /* can be NULL */ ambuildphasename_function ambuildphasename; /* can be NULL */ amvalidate_function amvalidate; +amcheckmembers_function amcheckmembers; /* can be NULL */ ambeginscan_function ambeginscan; amrescan_function amrescan; amgettuple_function amgettuple; /* can be NULL */ @@ -496,7 +497,47 @@ amvalidate (Oid opclassoid); the access method can reasonably do that. For example, this might include testing that all required support functions are provided. The amvalidate function must return false if the opclass is - invalid. Problems should be reported with ereport messages. + invalid. Problems should be reported with ereport + messages, typically at INFO level. + + + + +void +amcheckmembers (Oid opfamilyoid, +Oid opclassoid, +List *operators, +List *functions); + + Validate proposed operator and function members of an operator family, + so far as the access method can reasonably do that, and set their + dependency types if the default is not satisfactory. This is called + during CREATE OPERATOR CLASS and during + ALTER OPERATOR FAMILY ADD; in the latter + case opclassoid is InvalidOid. + The List arguments are lists + of OpFamilyMember structs, as
Re: Rethinking opclass member checks and dependency strength
> > 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.
Re: Rethinking opclass member checks and dependency strength
On Wed, Aug 7, 2019 at 7:28 PM Tom Lane 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
