Naming of network_ops vs. inet_ops for SP-GIST

Michael, please see below:

On Tue, Jan 24, 2023 at 12:44 PM PG Doc comments form <
noreply@postgresql.org> wrote:

The following documentation comment has been logged on the website:

Page: https://www.postgresql.org/docs/14/spgist-builtin-opclasses.html
Description:

I wanted to add an SP-GIST index for an inet field ip_address

In https://www.postgresql.org/docs/14/spgist-builtin-opclasses.html
network_ops is stated as the built-in opclass for (inet, inet)
[...]

I notice inet_ops, not network_ops, is mentioned in the docs for 13. Perhaps

it was renamed to network_ops in 15 but not 14?

I'm fairly certain this is a simple typo while performing some refactoring
work here:

https://github.com/postgres/postgres/commit/7a1cd5260aa20bc13aec8960a57904b5623d1830

doc/src/sgml/spgist.sgml
L105 + <entry valign="middle"
morerows="10"><literal>network_ops</literal></entry>
L185 - <entry><literal>inet_ops</literal></entry>

David J.

PG Doc comments form <noreply@postgresql.org> writes:

I wanted to add an SP-GIST index for an inet field ip_address
In https://www.postgresql.org/docs/14/spgist-builtin-opclasses.html
network_ops is stated as the built-in opclass for (inet, inet)
I entered the following command:
# create index concurrently ip_address_spgist_ban_by_ip on ban_by_ip using
spgist (ip_address network_ops);
ERROR: operator class "network_ops" does not exist for access method
"spgist"

Hmm. There's some confusion here, because network_ops is the name of
the operator *family*:

=# \dAf spgist
List of operator families
AM | Operator family | Applicable types
--------+-----------------+------------------
spgist | box_ops | box
spgist | kd_point_ops | point
spgist | network_ops | inet
spgist | poly_ops | polygon
spgist | quad_point_ops | point
spgist | range_ops | anyrange
spgist | text_ops | text
(7 rows)

but inet_ops is the name of the operator *class*:

n=# \dAc spgist
List of operator classes
AM | Input type | Storage type | Operator class | Default?
--------+------------+--------------+----------------+----------
spgist | anyrange | | range_ops | yes
spgist | box | | box_ops | yes
spgist | inet | | inet_ops | yes
spgist | point | | kd_point_ops | no
spgist | point | | quad_point_ops | yes
spgist | polygon | box | poly_ops | yes
spgist | text | | text_ops | yes
(7 rows)

This naming was evidently chosen to match btree, which has both
inet_ops and cidr_ops opclasses within its network_ops family.
spgist only supports inet_ops (and there's not really a reason
to change that, since it will in fact work for cidr too).
But you have to use the class name not the family name when
explicitly selecting an index's opclass.

I notice inet_ops, not network_ops, is mentioned in the docs for 13. Perhaps
it was renamed to network_ops in 15 but not 14?

Hmm, apparently somebody decided that the family name was more
appropriate to show here, since the operators are tied to an
opfamily not just an opclass. But the table header still says
"operator classes", so that's incorrect.

We could change the table header, but I'm not sure that that is a
useful direction to take, because people need to use the class name
for index creation but there are few cases where non-developers
need be concerned with family names.

I wonder whether we shouldn't just revert this table to
showing opclass names, and avert our eyes from the theoretical
inconsistency. Michael, looks like it was your 7a1cd5260
that changed it; what do you think?

regards, tom lane

This naming was evidently chosen to match btree,
which has both inet_ops and cidr_ops opclasses
within its network_ops family.
spgist only supports inet_ops

FWIW, the documentation for GIST has inet_ops in the equivalent table, so it was extra-confusing because I thought SP-GIST's inconsistency must have a reason - though I see now using '\dAf gist' suggests it's similar.
https://www.postgresql.org/docs/14/gist-builtin-opclasses.html

The index in question was replacing a default btree - we'd started using the table to block by range; when reviewing indexes, I found it wasn't used anymore. (I didn't know btree *had* cidr_ops, but that may be for the best, as it didn't help with 'CIDR contains IP'.)

Best regards,
--
Laurence 'GreenReaper' Parry
greenreaper.co.uk - Inkbunny.net

On Tue, Jan 24, 2023 at 03:22:44PM -0500, Tom Lane wrote:

I wonder whether we shouldn't just revert this table to
showing opclass names, and avert our eyes from the theoretical
inconsistency. Michael, looks like it was your 7a1cd5260
that changed it; what do you think?

Yes, the docs should be fixed here. The intention is not to show the
operator families but the names of the opclasses. I can only spot one
difference in SpGiST for network_ops -> inet_ops as of the report.
BRIN, GIN and GiST look to be clean after a second lookup.

I don't have a strong opinion about the naming inconsistency between
the opclass name and the opfamily name in this case, though, couldn't
it create more problems than actually fix something?

Anyway, attached is a patch for the docs. Thoughts?
--
Michael

Michael Paquier <michael@paquier.xyz> writes:

I don't have a strong opinion about the naming inconsistency between
the opclass name and the opfamily name in this case, though, couldn't
it create more problems than actually fix something?

Well, it's been like that from day one and people haven't complained.
I think changing it now would add more confusion than it subtracts.

Anyway, attached is a patch for the docs. Thoughts?

Works for me.

regards, tom lane

On Tue, Jan 24, 2023 at 10:38:28PM -0500, Tom Lane wrote:

Works for me.

Thanks for checking, applied that.
--
Michael