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