Обсуждение: Contains and is contained by operators of inet datatypes
Attached patch adds <@, @>, <<@, and @>> operator symbols for inet datatype to replace <<=, >>=, <<, and >>. <@ and @> symbols are used for containment for all datatypes except inet, particularly on the geometric types, arrays; cube, hstore, intaray, ltree extensions. <@ and @> symbols are standardised as on version 8.2 by Tom Lane at 2006 [1]. The previous symbols are left in-place but deprecated. The patch does exactly the same for inet datatypes. The << and >> are standard symbols for strictly left of and strictly right of operators. Those operators would also make sense for inet datatypes. If we make this change now; we can remove the symbols, and reuse them for new operators in distant future. The patch removes the recently committed SP-GiST index support for the existing operator symbols to give move reason to the users to use the new symbols. This change will also indirectly deprecate the undocumented non-transparent btree index support that works sometimes for some of the existing operators [2]. The patch includes changes on the regression tests and the documentation. I will add it to 2016-11 Commitfest. [1] https://www.postgresql.org/message-id/14277.1157304939@sss.pgh.pa.us [2] https://www.postgresql.org/message-id/389.1042992616@sss.pgh.pa.us
Вложения
Review - Applies and passes the test suite. - I think this is a good change since it increases the consistency of the operators. I also like the choice of <<@ and @>> since they feel intuitive to me. - I tested it and both old and new operators use the brin and gist indexes. - The new spgist index does not support the old deprecated operators, which is intentional. I do not have a strong opinion here either way but some people may find this surprising. - I am not convinced that your changes to the descriptions of the operators necessarily make things clearer. For example "is contained by and smaller network (subnet)" only mentions subnets and not IP-addresses. - Maybe change "deprecated and will eventually be removed." to "deprecated and may be removed in a future release.". I prefer that latter wording but I am fine with either. - Won't renaming the functions which implement risk breaking people's applications? While the new names are a bit nicer I am not sure it is worth doing. - The changes to the code look generally good. Andreas
> - I am not convinced that your changes to the descriptions of the operators > necessarily make things clearer. For example "is contained by and smaller > network (subnet)" only mentions subnets and not IP-addresses. I was trying to avoid confusion. <@ is the "contained by" operator which is also returning true when both sides are equal. We shouldn't continue calling <<@ also "contained by". I removed the "(subnet)" and "(supernet)" additions. Can you think of any better wording? > - Maybe change "deprecated and will eventually be removed." to "deprecated > and may be removed in a future release.". I prefer that latter wording but I > am fine with either. I copied that note from the Geometric Functions and Operators page. > - Won't renaming the functions which implement risk breaking people's > applications? While the new names are a bit nicer I am not sure it is worth > doing. You are right. I reverted that part. > - The changes to the code look generally good. Thank you for the review. New version is attached.
Вложения
On 11/13/2016 01:21 PM, Emre Hasegeli wrote: > Thank you for the review. New version is attached. Nice, I am fine with this version of the patch. Setting it to ready for committer! Andreas
Andreas Karlsson <andreas@proxel.se> writes:
> Emre Hasegeli wrote:
>>>>> Attached patch adds <@, @>, <<@, and @>> operator symbols for inet
>>>>> datatype to replace <<=, >>=, <<, and >>.
> Nice, I am fine with this version of the patch. Setting it to ready for
> committer!
I looked at this for awhile and TBH I am not very excited about it.
I am not sure it makes anything better, but I am sure it makes things
different. People tend not to like that.
The new names might be better if we were starting in a green field,
but in themselves they are not any more mnemonic than what we had, and
what we had has been there for a lot of years. Also, if we accept both
old names and new (which it seems like we'd have to), that creates new
opportunities for confusion, which is a cost that should not be
disregarded.
The original post proposed that we'd eventually get some benefit by
being able to repurpose << and >> to mean something else, but the
time scale over which that could happen is so long as to make it
unlikely to ever happen. I think we'd need to deprecate these names
for several years, then actually remove them and have nothing there for
a few years more, before we could safely install new operators that
take the same arguments but do something different. (For comparison's
sake, it took us five years to go from deprecating => as a user operator
to starting to use it as parameter naming syntax ... and that was a
case where conflicting use could be expected to throw an error, not
silently misbehave, so we could force it with little risk of silently
breaking peoples' applications. To repurpose << and >> in this way
we would need to move much slower.)
I'm inclined to think we should just reject this patch. I'm certainly not
going to commit it without seeing positive votes from multiple people.
regards, tom lane
> The new names might be better if we were starting in a green field, > but in themselves they are not any more mnemonic than what we had, and > what we had has been there for a lot of years. Also, if we accept both > old names and new (which it seems like we'd have to), that creates new > opportunities for confusion, which is a cost that should not be > disregarded. This is true for existing users of those operators. New names are much easier to get by the new users. We are using them on all other datatypes. Datatypes like JSON is more popular than inet. Many people already know what <@ and @> mean. > The original post proposed that we'd eventually get some benefit by > being able to repurpose << and >> to mean something else, but the > time scale over which that could happen is so long as to make it > unlikely to ever happen. I think we will immediately get benefit from the new operators because of other reasons. Repurposing them in far future was a minor point. I though having a long term plan on this minor point is better than having no plan. > I'm inclined to think we should just reject this patch. I'm certainly not > going to commit it without seeing positive votes from multiple people. It is a fair position. Anybody care to vote?
On 11/17/2016 11:14 PM, Tom Lane wrote: > The original post proposed that we'd eventually get some benefit by > being able to repurpose << and >> to mean something else, but the > time scale over which that could happen is so long as to make it > unlikely to ever happen. I think we'd need to deprecate these names > for several years, then actually remove them and have nothing there for > a few years more, before we could safely install new operators that > take the same arguments but do something different. (For comparison's > sake, it took us five years to go from deprecating => as a user operator > to starting to use it as parameter naming syntax ... and that was a > case where conflicting use could be expected to throw an error, not > silently misbehave, so we could force it with little risk of silently > breaking peoples' applications. To repurpose << and >> in this way > we would need to move much slower.) I agree. The value in re-purposing them is pretty low given the long time scales needed before that can be done. > I'm inclined to think we should just reject this patch. I'm certainly not > going to commit it without seeing positive votes from multiple people. Given that I reviewed it I think you already have my vote on this. I like the patch because it means less operators to remember for me as a PostgreSQL user. And at least for me inet is a rarely used type compared to hstore, json and range types which all use @> and <@. Andreas
On Mon, Nov 21, 2016 at 7:57 AM, Andreas Karlsson <andreas@proxel.se> wrote: > I like the patch because it means less operators to remember for me as a > PostgreSQL user. And at least for me inet is a rarely used type compared to > hstore, json and range types which all use @> and <@. I agree that it would be nice to make the choice of operator names more consistent. I don't know if doing so will please more or fewer people than it annoys. I do not like this bit from the original post: EH> The patch removes the recently committed SP-GiST index support for the EH> existing operator symbols to give move reason to the users to use the EH> new symbols. That seems like the rough equivalent of throwing a wrench into the datacenter's backup generator to "encourage" them to maintain two separate and independent backup generators. If we're going to add the more-standard operator names as synonyms for the existing operator names, let's do precisely that and no more. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
> I do not like this bit from the original post: > > EH> The patch removes the recently committed SP-GiST index support for the > EH> existing operator symbols to give move reason to the users to use the > EH> new symbols. I reverted this part. The new version of the patch is attached.
Вложения
Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Nov 21, 2016 at 7:57 AM, Andreas Karlsson <andreas@proxel.se> wrote:
>> I like the patch because it means less operators to remember for me as a
>> PostgreSQL user. And at least for me inet is a rarely used type compared to
>> hstore, json and range types which all use @> and <@.
> I agree that it would be nice to make the choice of operator names
> more consistent. I don't know if doing so will please more or fewer
> people than it annoys.
Given the lack of support for this idea from the rest of the community,
I think it's time to reject this patch and move on. The inconsistency
is unfortunate but it does not seem worth the costs of changing.
regards, tom lane