Re: factorial function/phase out postfix operators?
John Naylor writes: > I believe it's actually "lower than Op", and since POSTFIXOP is gone > it doesn't seem to matter how low it is. In fact, I found that the > lines with INDENT and UNBOUNDED now work as the lowest precedence > declarations. Maybe that's worth something? > Following on Peter E.'s example upthread, GENERATED can be removed > from precedence, and I also found the same is true for PRESERVE and > STRIP_P. Now that the main patch is pushed, I went back to revisit this precedence issue. I'm afraid to move the precedence of IDENT as much as you suggest here. The comment for opt_existing_window_name says that it's expecting the precedence of IDENT to be just below that of Op. If there's daylight in between, that could result in funny behavior for use of some of the unreserved words with other precedence levels in this context. However, I concur that we ought to be able to remove the explicit precedences for GENERATED, NULL_P, PRESERVE, and STRIP_P, so I did that. An interesting point is that it's actually possible to remove the precedence declaration for IDENT itself (at least, that does not create any bison errors; I did not do any behavioral testing). I believe what we had that for originally was to control the precedence behavior of the "target_el: a_expr IDENT" rule, and now that that rule doesn't end with IDENT, its behavior isn't governed by that. But I think we're best off to keep the precedence assignment, as a place to hang the precedences of PARTITION etc. regards, tom lane
Re: factorial function/phase out postfix operators?
On Fri, Sep 18, 2020 at 4:48 PM Tom Lane wrote: > Pushed with the discussed terminological changes and some other > fooling about, including fixing the documentation. Awesome. Thanks! -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
Pushed with the discussed terminological changes and some other fooling about, including fixing the documentation. regards, tom lane
Re: factorial function/phase out postfix operators?
Robert Haas writes:
> On Fri, Sep 18, 2020 at 2:11 PM Tom Lane wrote:
>> What I now propose is to add two output columns:
>>
>> barelabel bool (t or f, obviously)
>> baredesc text ("can be bare label" or "requires AS", possibly localized)
> That might be over-engineered in a vacuum, but it seems like it may be
> cleaner to stick with the existing precedent than to diverge from it.
Yeah, my recollection of the pg_get_keywords design is that we couldn't
agree on whether to emit a machine-friendly description or a
human-friendly one, so we compromised by doing both :-(. But the same
factors exist with this addition --- you can make an argument for
preferring either boolean or text output.
regards, tom lane
Re: factorial function/phase out postfix operators?
On Fri, Sep 18, 2020 at 2:11 PM Tom Lane wrote:
> After re-reading the description of pg_get_keywords, I was reminded that
> what it outputs now is intended to provide both a machine-friendly
> description of the keyword category ("catcode") and a human-friendly
> description ("catdesc"). So we really should do likewise for the
> label property. What I now propose is to add two output columns:
>
> barelabel bool (t or f, obviously)
> baredesc text ("can be bare label" or "requires AS", possibly localized)
That might be over-engineered in a vacuum, but it seems like it may be
cleaner to stick with the existing precedent than to diverge from it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
Robert Haas writes:
> On Fri, Sep 18, 2020 at 11:29 AM Tom Lane wrote:
>> As for what to expose in pg_get_keywords, I think something like
>> "label_requires_as bool" would be immediately understandable.
>> If you really want it to be an enum sort of thing, maybe the output
>> column title could be "collabel" with values "bare" or "requires_AS".
> It's sort of possible to be confused by "label requires as" since "as"
> is being used as a known but isn't really one generally speaking, but
> we can't very well quote it so I don't know how to make it more clear.
After re-reading the description of pg_get_keywords, I was reminded that
what it outputs now is intended to provide both a machine-friendly
description of the keyword category ("catcode") and a human-friendly
description ("catdesc"). So we really should do likewise for the
label property. What I now propose is to add two output columns:
barelabel bool (t or f, obviously)
baredesc text ("can be bare label" or "requires AS", possibly localized)
Feel free to bikeshed on those details.
regards, tom lane
Re: factorial function/phase out postfix operators?
On Fri, Sep 18, 2020 at 11:29 AM Tom Lane wrote: > I confess to not having paid very close attention to this thread > lately, but the last I'd noticed the terminology proposed for > internal use was "bare column label", which I think is much better. I agree. > As for what to expose in pg_get_keywords, I think something like > "label_requires_as bool" would be immediately understandable. > If you really want it to be an enum sort of thing, maybe the output > column title could be "collabel" with values "bare" or "requires_AS". It's sort of possible to be confused by "label requires as" since "as" is being used as a known but isn't really one generally speaking, but we can't very well quote it so I don't know how to make it more clear. > So I'm thinking about making these changes in gram.y: > > ImplicitAlias -> BareColLabel > implicit_alias_keyword -> bare_label_keyword > > and corresponding terminology changes elsewhere. +1. Thanks for picking this up; I am pretty excited about this. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
> On Sep 18, 2020, at 8:29 AM, Tom Lane wrote: > > So I've finished up applying 0001 and started to look at 0002 > ... and I find the terminology you've chosen to be just really > opaque and confusing. "aliastype" being "implicit" or "explicit" > is not going to make any sense to anyone until they read the > manual, and it probably still won't make sense after that. > > In the first place, the terminology we use for these things > is usually "column label", not "alias"; see e.g. > https://www.postgresql.org/docs/devel/queries-select-lists.html#QUERIES-COLUMN-LABELS > Likewise, gram.y itself refers to the construct as a ColLabel. > Aliases are things that appear in the FROM clause. > > In the second place, "implicit" vs "explicit" just doesn't make > any sense to me. You could maybe say that the AS is implicit > when you omit it, but the column label is surely not implicit; > it's right there where you wrote it. > > I confess to not having paid very close attention to this thread > lately, but the last I'd noticed the terminology proposed for > internal use was "bare column label", which I think is much better. > As for what to expose in pg_get_keywords, I think something like > "label_requires_as bool" would be immediately understandable. > If you really want it to be an enum sort of thing, maybe the output > column title could be "collabel" with values "bare" or "requires_AS". > > So I'm thinking about making these changes in gram.y: > > ImplicitAlias -> BareColLabel > implicit_alias_keyword -> bare_label_keyword > > and corresponding terminology changes elsewhere. That sounds ok to me. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
So I've finished up applying 0001 and started to look at 0002 ... and I find the terminology you've chosen to be just really opaque and confusing. "aliastype" being "implicit" or "explicit" is not going to make any sense to anyone until they read the manual, and it probably still won't make sense after that. In the first place, the terminology we use for these things is usually "column label", not "alias"; see e.g. https://www.postgresql.org/docs/devel/queries-select-lists.html#QUERIES-COLUMN-LABELS Likewise, gram.y itself refers to the construct as a ColLabel. Aliases are things that appear in the FROM clause. In the second place, "implicit" vs "explicit" just doesn't make any sense to me. You could maybe say that the AS is implicit when you omit it, but the column label is surely not implicit; it's right there where you wrote it. I confess to not having paid very close attention to this thread lately, but the last I'd noticed the terminology proposed for internal use was "bare column label", which I think is much better. As for what to expose in pg_get_keywords, I think something like "label_requires_as bool" would be immediately understandable. If you really want it to be an enum sort of thing, maybe the output column title could be "collabel" with values "bare" or "requires_AS". So I'm thinking about making these changes in gram.y: ImplicitAlias -> BareColLabel implicit_alias_keyword -> bare_label_keyword and corresponding terminology changes elsewhere. Thoughts? regards, tom lane
Re: factorial function/phase out postfix operators?
Robert Haas writes: > I'm not sure who is going to commit this work, and that person may > have a different preference than me. However, if it's me, I'd like to > see the removal of the existing postfix operators broken off into its > own patch, separate from the removal of the underlying facility to > have postfix operators. I've pushed a subset of the v7-0001 patch to meet Robert's preference. Continuing to look at the rest of it. regards, tom lane
Re: factorial function/phase out postfix operators?
Mark Dilger writes: > On Sep 11, 2020, at 12:54 PM, Robert Haas wrote: >> On Fri, Sep 11, 2020 at 3:23 PM Mark Dilger >> wrote: >>> Another option would be to have pg_dump take a strictness mode option. I >>> don't think the option should have anything to do with postfix operators >>> specifically, but be more general like --dump-incompatible-objects vs. >>> --omit-incompatible-objects vs. --error-on-incompatible-objects vs. >>> --do-your-best-to-fixup-incompatible-objects, with one of those being the >>> default (and with all of them having better names). If >>> --error-on-incompatible-objects were the default, that would behave as >>> Robert recommended upthread. >>> I can totally see an objection to the added complexity of such options, so >>> I'm really just putting this out on the list for comment. >> I'm not opposed to Tom's proposal. I just wanted to raise the issue >> for discussion. > Ah, ok. I don't feel any need for changes, either. I'll leave the > patch as it stands now. We're in violent agreement it seems. At some point it might be worth doing something like what Mark suggests above, but this patch shouldn't be tasked with it. In any case, since pg_dump does not know what the target server version really is, it's going to be hard for it to authoritatively distinguish what will work or not. regards, tom lane
Re: factorial function/phase out postfix operators?
> On Sep 11, 2020, at 12:54 PM, Robert Haas wrote: > > On Fri, Sep 11, 2020 at 3:23 PM Mark Dilger > wrote: >> Another option would be to have pg_dump take a strictness mode option. I >> don't think the option should have anything to do with postfix operators >> specifically, but be more general like --dump-incompatible-objects vs. >> --omit-incompatible-objects vs. --error-on-incompatible-objects vs. >> --do-your-best-to-fixup-incompatible-objects, with one of those being the >> default (and with all of them having better names). If >> --error-on-incompatible-objects were the default, that would behave as >> Robert recommended upthread. >> >> I can totally see an objection to the added complexity of such options, so >> I'm really just putting this out on the list for comment. > > I'm not opposed to Tom's proposal. I just wanted to raise the issue > for discussion. Ah, ok. I don't feel any need for changes, either. I'll leave the patch as it stands now. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
On Fri, Sep 11, 2020 at 3:23 PM Mark Dilger wrote: > Another option would be to have pg_dump take a strictness mode option. I > don't think the option should have anything to do with postfix operators > specifically, but be more general like --dump-incompatible-objects vs. > --omit-incompatible-objects vs. --error-on-incompatible-objects vs. > --do-your-best-to-fixup-incompatible-objects, with one of those being the > default (and with all of them having better names). If > --error-on-incompatible-objects were the default, that would behave as Robert > recommended upthread. > > I can totally see an objection to the added complexity of such options, so > I'm really just putting this out on the list for comment. I'm not opposed to Tom's proposal. I just wanted to raise the issue for discussion. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
> On Sep 11, 2020, at 11:25 AM, Tom Lane wrote: > > Mark Dilger writes: >> On Sep 11, 2020, at 8:36 AM, Tom Lane wrote: >>> My inclination is to simply not change pg_dump. There is no need to break >>> the use-case of loading the output back into the server version it came >>> from, if we don't have to. If the output is getting loaded into a server >>> that lacks postfix operators, that server can throw the error. There's no >>> real gain in having pg_dump prejudge the issue. > >> I think some kind of indication that the dump won't be loadable is >> useful if they're planning to move the dump file across an expensive >> link, or if they intend to blow away the old data directory to make room >> for the new. Whether that indication should be in the form of a warning >> or an error is less clear to me. > > I think definitely not an error, because that breaks a plausible (even if > not recommended) use-case. > >> Whatever we do here, I think it sets a precedent for how such situations >> are handled in the future, so maybe focusing overmuch on the postfix >> operator issue is less helpful than on the broader concept. What, for >> example, would we do if we someday dropped GiST support? > > I'm not sure that there is or should be a one-size-fits-all policy. > We do actually have multiple precedents already: > > * DefineIndex substitutes "gist" for "rtree" to allow transparent updating > of dumps from DBs that used the old rtree AM. > > * Up till very recently (84eca14bc), ResolveOpClass had similar hacks to > substitute for old opclass names. > > * bb03010b9 and e58a59975 got rid of other server-side hacks for > converting old dump files. > > So generally the preference is to make the server deal with conversion > issues; and this must be so, since what you have to work with may be a > dump taken with an old pg_dump. In this case, though, it doesn't seem > like there's any plausible way for the server to translate old DDL. > > As for the pg_dump side, aside from the WITH OIDS precedent you mentioned, > there was till recently (d9fa17aa7) code to deal with unconvertible > pre-7.1 aggregates. That code issued a pg_log_warning and then ignored > (didn't dump) the aggregate. I think it didn't have much choice about > the latter step because, if memory serves, there simply wasn't any way to > represent those old aggregates in the new CREATE AGGREGATE syntax; so we > couldn't leave it to the server to decide whether to throw error or not. > (It's also possible, given how far back that was, that we simply weren't > being very considerate of upgrade issues. It's old enough that I would > not take it as great precedent. But it is a precedent.) > > The behavior of WITH OIDS is to issue a pg_log_warning and then ignore > the property. I do not much care for this, although I see the point that > we don't want to stick WITH OIDS into the CREATE TABLE because then the > CREATE would fail, leaving the dump completely unusable on newer servers. > My choice would have been to write CREATE TABLE without that option and > then add ALTER TABLE ... WITH OIDS. In this way the dump script does > what it should when restoring into an old server, while if you load into > a new server you hear about it --- and you can ignore the error if you > want. > > I think the right thing for postfix operators is probably to issue > pg_log_warning and then dump the object anyway. That happens to be the patch behavior as it stands now. Another option would be to have pg_dump take a strictness mode option. I don't think the option should have anything to do with postfix operators specifically, but be more general like --dump-incompatible-objects vs. --omit-incompatible-objects vs. --error-on-incompatible-objects vs. --do-your-best-to-fixup-incompatible-objects, with one of those being the default (and with all of them having better names). If --error-on-incompatible-objects were the default, that would behave as Robert recommended upthread. I can totally see an objection to the added complexity of such options, so I'm really just putting this out on the list for comment. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
Mark Dilger writes: > On Sep 11, 2020, at 8:36 AM, Tom Lane wrote: >> My inclination is to simply not change pg_dump. There is no need to break >> the use-case of loading the output back into the server version it came >> from, if we don't have to. If the output is getting loaded into a server >> that lacks postfix operators, that server can throw the error. There's no >> real gain in having pg_dump prejudge the issue. > I think some kind of indication that the dump won't be loadable is > useful if they're planning to move the dump file across an expensive > link, or if they intend to blow away the old data directory to make room > for the new. Whether that indication should be in the form of a warning > or an error is less clear to me. I think definitely not an error, because that breaks a plausible (even if not recommended) use-case. > Whatever we do here, I think it sets a precedent for how such situations > are handled in the future, so maybe focusing overmuch on the postfix > operator issue is less helpful than on the broader concept. What, for > example, would we do if we someday dropped GiST support? I'm not sure that there is or should be a one-size-fits-all policy. We do actually have multiple precedents already: * DefineIndex substitutes "gist" for "rtree" to allow transparent updating of dumps from DBs that used the old rtree AM. * Up till very recently (84eca14bc), ResolveOpClass had similar hacks to substitute for old opclass names. * bb03010b9 and e58a59975 got rid of other server-side hacks for converting old dump files. So generally the preference is to make the server deal with conversion issues; and this must be so, since what you have to work with may be a dump taken with an old pg_dump. In this case, though, it doesn't seem like there's any plausible way for the server to translate old DDL. As for the pg_dump side, aside from the WITH OIDS precedent you mentioned, there was till recently (d9fa17aa7) code to deal with unconvertible pre-7.1 aggregates. That code issued a pg_log_warning and then ignored (didn't dump) the aggregate. I think it didn't have much choice about the latter step because, if memory serves, there simply wasn't any way to represent those old aggregates in the new CREATE AGGREGATE syntax; so we couldn't leave it to the server to decide whether to throw error or not. (It's also possible, given how far back that was, that we simply weren't being very considerate of upgrade issues. It's old enough that I would not take it as great precedent. But it is a precedent.) The behavior of WITH OIDS is to issue a pg_log_warning and then ignore the property. I do not much care for this, although I see the point that we don't want to stick WITH OIDS into the CREATE TABLE because then the CREATE would fail, leaving the dump completely unusable on newer servers. My choice would have been to write CREATE TABLE without that option and then add ALTER TABLE ... WITH OIDS. In this way the dump script does what it should when restoring into an old server, while if you load into a new server you hear about it --- and you can ignore the error if you want. I think the right thing for postfix operators is probably to issue pg_log_warning and then dump the object anyway. regards, tom lane
Re: factorial function/phase out postfix operators?
> On Sep 11, 2020, at 8:36 AM, Tom Lane wrote: > > Robert Haas writes: >> On Thu, Sep 3, 2020 at 11:50 AM Mark Dilger >> wrote: >>> Since newer pg_dump binaries can be used to dump data from older servers, >>> and since users might then load that dump back into an older server, I >>> think doing anything stronger than a pg_log_warning() would be incorrect. >>> I did not find precedents under comparable circumstances for taking >>> stronger actions than pg_log_warning. I assume we can't, for example, omit >>> the operator from the dump, nor can we abort the process. > >> I'm not sure that this is the right solution. Generally, the >> recommendation is that you should use the pg_dump that corresponds to >> the server version where you want to do the reload, so if you're >> hoping to dump 9.6 and restore on 11, you should be using the pg_dump >> from 11, not 14. So my thought would be that if there are user-defined >> postfix operators, pg_dump ought to error out. However, that could be >> inconvenient for people who are using pg_dump in ways that are maybe >> not what we would recommend but which may happen to work but for this >> issue, so I'm not sure. On the third hand, though, we think that there >> are very few user-defined postfix operators out there, so if we just >> give an error, we probably won't be inconveniencing many people. > > My inclination is to simply not change pg_dump. There is no need to break > the use-case of loading the output back into the server version it came > from, if we don't have to. If the output is getting loaded into a server > that lacks postfix operators, that server can throw the error. There's no > real gain in having pg_dump prejudge the issue. I think some kind of indication that the dump won't be loadable is useful if they're planning to move the dump file across an expensive link, or if they intend to blow away the old data directory to make room for the new. Whether that indication should be in the form of a warning or an error is less clear to me. Whatever we do here, I think it sets a precedent for how such situations are handled in the future, so maybe focusing overmuch on the postfix operator issue is less helpful than on the broader concept. What, for example, would we do if we someday dropped GiST support? Print a warning when dumping a database with GiST indexes? Omit the indexes? Abort the dump? The docs at https://www.postgresql.org/docs/12/app-pgdump.html say: > Because pg_dump is used to transfer data to newer versions of PostgreSQL, the > output of pg_dump can be expected to load into PostgreSQL server versions > newer than pg_dump's version. > Also, it is not guaranteed that pg_dump's output can be loaded into a server > of an older major version — not even if the dump was taken from a server of > that version. I think somewhere around here the docs need to call out what happens when the older major version supported a feature that has been dropped from the newer major version. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
Robert Haas writes: > On Thu, Sep 3, 2020 at 11:50 AM Mark Dilger > wrote: >> Since newer pg_dump binaries can be used to dump data from older servers, >> and since users might then load that dump back into an older server, I think >> doing anything stronger than a pg_log_warning() would be incorrect. I did >> not find precedents under comparable circumstances for taking stronger >> actions than pg_log_warning. I assume we can't, for example, omit the >> operator from the dump, nor can we abort the process. > I'm not sure that this is the right solution. Generally, the > recommendation is that you should use the pg_dump that corresponds to > the server version where you want to do the reload, so if you're > hoping to dump 9.6 and restore on 11, you should be using the pg_dump > from 11, not 14. So my thought would be that if there are user-defined > postfix operators, pg_dump ought to error out. However, that could be > inconvenient for people who are using pg_dump in ways that are maybe > not what we would recommend but which may happen to work but for this > issue, so I'm not sure. On the third hand, though, we think that there > are very few user-defined postfix operators out there, so if we just > give an error, we probably won't be inconveniencing many people. My inclination is to simply not change pg_dump. There is no need to break the use-case of loading the output back into the server version it came from, if we don't have to. If the output is getting loaded into a server that lacks postfix operators, that server can throw the error. There's no real gain in having pg_dump prejudge the issue. regards, tom lane
Re: factorial function/phase out postfix operators?
On Thu, Sep 3, 2020 at 11:50 AM Mark Dilger wrote: > Since newer pg_dump binaries can be used to dump data from older servers, and > since users might then load that dump back into an older server, I think > doing anything stronger than a pg_log_warning() would be incorrect. I did > not find precedents under comparable circumstances for taking stronger > actions than pg_log_warning. I assume we can't, for example, omit the > operator from the dump, nor can we abort the process. I'm not sure that this is the right solution. Generally, the recommendation is that you should use the pg_dump that corresponds to the server version where you want to do the reload, so if you're hoping to dump 9.6 and restore on 11, you should be using the pg_dump from 11, not 14. So my thought would be that if there are user-defined postfix operators, pg_dump ought to error out. However, that could be inconvenient for people who are using pg_dump in ways that are maybe not what we would recommend but which may happen to work but for this issue, so I'm not sure. On the third hand, though, we think that there are very few user-defined postfix operators out there, so if we just give an error, we probably won't be inconveniencing many people. I'm not sure who is going to commit this work, and that person may have a different preference than me. However, if it's me, I'd like to see the removal of the existing postfix operators broken off into its own patch, separate from the removal of the underlying facility to have postfix operators. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
On Thu, Sep 3, 2020 at 11:50 AM Mark Dilger wrote: > [v7] Ok, I've marked it ready for committer. -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: factorial function/phase out postfix operators?
On Tue, Sep 1, 2020 at 10:00 PM Mark Dilger wrote: > > Some changes were made on another thread [1] for the deprecation notices, > committed recently by Tom, and I think this patch set is compatible with what > was done there. This patch set is intended for commit against master, > targeted for PostgreSQL 14, so the deprecation notices are removed along with > the things that were deprecated. The references to right-unary operators > that you call out, above, have been removed. Hi Mark, Looks good. Just a couple things I found in 0001: The factorial operators should now be removed from func.sgml. For pg_dump, should we issue a pg_log_warning() (or stronger) somewhere if user-defined postfix operators are found? I'm looking at the example of "WITH OIDS" in pg_dump.c. Nitpick: these can be removed, since we already test factorial() in this file: -SELECT 4!; -SELECT !!3; +SELECT factorial(4); +SELECT factorial(3); -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: factorial function/phase out postfix operators?
On Fri, Aug 28, 2020 at 11:00 AM Robert Haas wrote: > On Fri, Aug 28, 2020 at 4:44 AM John Naylor > wrote: > > On Thu, Aug 27, 2020 at 5:04 PM Tom Lane wrote: > > > I'm a bit inclined to kill them both off and standardize on factorial() > > > (not numeric_fac). > > > > +1 > > Here's a modified version of John's patch that also describes ! and !! > as deprecated. It looked too wordy to me to recommend what should be > used instead, so I have not done that. > > Comments? Never mind, I see there's a new thread for this. Sorry for the noise. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
On Fri, Aug 28, 2020 at 4:44 AM John Naylor wrote: > On Thu, Aug 27, 2020 at 5:04 PM Tom Lane wrote: > > I'm a bit inclined to kill them both off and standardize on factorial() > > (not numeric_fac). > > +1 Here's a modified version of John's patch that also describes ! and !! as deprecated. It looked too wordy to me to recommend what should be used instead, so I have not done that. Comments? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company postfix-and-factorial-deprecate.patch Description: Binary data
Re: factorial function/phase out postfix operators?
On Wed, Aug 26, 2020 at 6:57 PM Mark Dilger wrote: > I don't have any problem with the changes you made in your patch, but > building on your changes I also found that the following cleanup causes no > apparent problems: > > -%nonassoc UNBOUNDED /* ideally should have same > precedence as IDENT */ > -%nonassoc IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE > ROLLUP > +%nonassoc UNBOUNDED IDENT > +%nonassoc PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP Thinking about this some more, I don't think we don't need to do any precedence refactoring in order to apply the functional change of these patches. We could leave that for follow-on patches once we figure out the best way forward, which could take some time. -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: factorial function/phase out postfix operators?
On Thu, Aug 27, 2020 at 5:04 PM Tom Lane wrote: > I'm a bit inclined to kill them both off and standardize on factorial() > (not numeric_fac). +1 -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: factorial function/phase out postfix operators?
On Thu, Aug 27, 2020 at 10:04 AM Tom Lane wrote: > Well, the !! operator itself has been "deprecated" for a long time: > > regression=# \do+ !! > List of operators >Schema | Name | Left arg type | Right arg type | Result type | Function > |Description > +--+---++-+-+--- > pg_catalog | !! | | bigint | numeric | > numeric_fac | deprecated, use ! instead > pg_catalog | !! | | tsquery| tsquery | > tsquery_not | NOT tsquery > (2 rows) > > I'm a bit inclined to kill them both off and standardize on factorial() > (not numeric_fac). Works for me. !! hasn't been marked as deprecated in the documentation, only the operator comment, which probably not many people look at. But I don't see a problem updating the documentation now to say: - !! is going away, use factorial() - ! is going away, use factorial() - postfix operators are going away -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
> On Aug 27, 2020, at 7:04 AM, Tom Lane wrote: > > Robert Haas writes: >> Yeah, that looks like a good spot. I think we should also add >> something to the documentation of the factorial operator, mentioning >> that it will be going away. Perhaps we can advise people to write !!3 >> instead of 3! for forward-compatibility, or maybe we should instead >> suggest numeric_fac(3). > > Well, the !! operator itself has been "deprecated" for a long time: > > regression=# \do+ !! > List of operators > Schema | Name | Left arg type | Right arg type | Result type | Function > |Description > +--+---++-+-+--- > pg_catalog | !! | | bigint | numeric | > numeric_fac | deprecated, use ! instead > pg_catalog | !! | | tsquery| tsquery | > tsquery_not | NOT tsquery > (2 rows) > > I'm a bit inclined to kill them both off and standardize on factorial() > (not numeric_fac). > > regards, tom lane Just for historical context, it seems that when you committed 908ab80286401bb20a519fa7dc7a837631f20369 in 2011, you were choosing one operator per underlying proc to be the canonical operator name, and deprecating all other operators based on the same proc. You chose postfix ! as the canonical operator for numeric_fac and deprecated prefix !!, but I think I can infer from that commit that if postfix ! did not exist, prefix !! would have been the canonical operator and would not have been deprecated. The main reason I did not remove prefix !! in this patch series is that the patch is about removing postfix operator support, and so it seemed off topic. But if there is general agreement to remove prefix !!, I'll put that in the next patch. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
Robert Haas writes: > Yeah, that looks like a good spot. I think we should also add > something to the documentation of the factorial operator, mentioning > that it will be going away. Perhaps we can advise people to write !!3 > instead of 3! for forward-compatibility, or maybe we should instead > suggest numeric_fac(3). Well, the !! operator itself has been "deprecated" for a long time: regression=# \do+ !! List of operators Schema | Name | Left arg type | Right arg type | Result type | Function |Description +--+---++-+-+--- pg_catalog | !! | | bigint | numeric | numeric_fac | deprecated, use ! instead pg_catalog | !! | | tsquery| tsquery | tsquery_not | NOT tsquery (2 rows) I'm a bit inclined to kill them both off and standardize on factorial() (not numeric_fac). regards, tom lane
Re: factorial function/phase out postfix operators?
On Thu, Aug 27, 2020 at 7:12 AM John Naylor wrote: > Well, for starters it'll say the obvious, but since we have a concrete > timeframe, maybe a tag to make it more visible, like in the > attached, compressed to avoid confusing the cfbot. Yeah, that looks like a good spot. I think we should also add something to the documentation of the factorial operator, mentioning that it will be going away. Perhaps we can advise people to write !!3 instead of 3! for forward-compatibility, or maybe we should instead suggest numeric_fac(3). -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
On Wed, Aug 26, 2020 at 8:55 PM Robert Haas wrote: > > On Wed, Aug 26, 2020 at 11:57 AM Mark Dilger > wrote: > > I wonder if we can get more comments for or against this patch, at least in > > principle, in the very near future, to help determine whether the > > deprecation notices should go into v13? > > Speaking of that, has somebody written a specific patch for that? > Like, exactly what are we proposing that this deprecation warning is > going to say? Well, for starters it'll say the obvious, but since we have a concrete timeframe, maybe a tag to make it more visible, like in the attached, compressed to avoid confusing the cfbot. -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services postfix-depr.patch.gz Description: GNU Zip compressed data
Re: factorial function/phase out postfix operators?
On Wed, Aug 26, 2020 at 6:57 PM Mark Dilger wrote: > > > > > On Aug 26, 2020, at 6:33 AM, John Naylor > > wrote: > > > and since POSTFIXOP is gone > > it doesn't seem to matter how low it is. In fact, I found that the > > lines with INDENT and UNBOUNDED now work as the lowest precedence > > declarations. Maybe that's worth something? > > > > Following on Peter E.'s example upthread, GENERATED can be removed > > from precedence, and I also found the same is true for PRESERVE and > > STRIP_P. > > > > I've attached a patch which applies on top of 0001 to demonstrate > > this. There might possibly still be syntax errors for things not > > covered in the regression test, but there are no s/r conflicts at > > least. > > I don't have any problem with the changes you made in your patch, but > building on your changes I also found that the following cleanup causes no > apparent problems: > > -%nonassoc UNBOUNDED /* ideally should have same > precedence as IDENT */ > -%nonassoc IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE > ROLLUP > +%nonassoc UNBOUNDED IDENT > +%nonassoc PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP > > Which does what the old comment apparently wanted. This changes the context of the comment at the top of the block: * To support target_el without AS, we must give IDENT an explicit priority * lower than Op. We can safely assign the same priority to various * unreserved keywords as needed to resolve ambiguities (this can't have any This also works: -%nonassoc UNBOUNDED /* ideally should have same precedence as IDENT */ -%nonassoc IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP +%nonassoc UNBOUNDED IDENT PARTITION RANGE ROWS GROUPS CUBE ROLLUP +%nonassoc PRECEDING FOLLOWING Not sure if either is better. Some additional input would be good here. While looking for a place to put a v13 deprecation notice, I found some more places in the docs which need updating: ref/create_operator.sgml "At least one of LEFTARG and RIGHTARG must be defined. For binary operators, both must be defined. For right unary operators, only LEFTARG should be defined, while for left unary operators only RIGHTARG should be defined." ref/create_opclass.sgml "In an OPERATOR clause, the operand data type(s) of the operator, or NONE to signify a left-unary or right-unary operator." -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: factorial function/phase out postfix operators?
On Wed, Aug 26, 2020 at 11:57 AM Mark Dilger wrote: > I wonder if we can get more comments for or against this patch, at least in > principle, in the very near future, to help determine whether the deprecation > notices should go into v13? Speaking of that, has somebody written a specific patch for that? Like, exactly what are we proposing that this deprecation warning is going to say? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
On Wed, Aug 26, 2020 at 6:12 AM Mark Dilger
wrote:
>
> The construction colname AS colalias brings to mind the words "pseudonym" and
> "alias". The distinction we're trying to draw here is between implicit
> pseudoyms and explicit ones, but "alias" is shorter and simpler, so I like
> that better than "pseudonym". Both are labels, so adding "label" to the name
> doesn't really get us anything. The constructions "implicit alias" vs.
> "explicit alias" seem to me to be an improvement, along with their other
> forms like "ImplicitAlias", or "implicit_alias", etc., so I've used those in
> version 4.
> The word "status" here really means something like "plicity" (implict vs.
> explicit), but "plicity" isn't a word, so I used "aliastype" instead.
Seems fine.
> A list of user defined postfix operators is in the file:
> postfix_ops.txt
>
> Failure, exiting
>
>
> With the contents of postfix_ops.txt:
>
> In database: regression
> (oid=27113) public.#@# (pg_catalog.int8)
> (oid=27114) public.#%# (pg_catalog.int8)
>
> which should be enough for a user to identify which operator is meant. I
> just invented that format. Let me know if there is a preferred way to lay
> out that information.
Not sure if there's a precedent here, and seems fine to me.
+ /*
+ * If neither argument is specified, do not mention postfix operators, as
+ * the user is unlikely to have meant to create one. It is more likely
+ * they simply neglected to mention the args.
+ */
if (!OidIsValid(typeId1) && !OidIsValid(typeId2))
ereport(ERROR,
(errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("at least one of leftarg or rightarg must be specified")));
+ errmsg("operator arguments must be specified")));
+
+ /*
+ * But if only the right arg is missing, they probably do intend to create
+ * a postfix operator, so give them a hint about why that does not work.
+ */
+ if (!OidIsValid(typeId2))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator right argument must be specified"),
+ errhint("Postfix operators are not supported.")));
This is just a nitpick -- I think the comments in this section would
flow better if order of checks were reversed, although the code might
not. I don't feel too strongly about it.
- * between POSTFIXOP and Op. We can safely assign the same priority to
- * various unreserved keywords as needed to resolve ambiguities (this can't
- * have any bad effects since obviously the keywords will still behave the
- * same as if they weren't keywords). We need to do this:
+ * greater than Op. We can safely assign the same priority to various
+ * unreserved keywords as needed to resolve ambiguities (this can't have any
+ * bad effects since obviously the keywords will still behave the same as if
+ * they weren't keywords). We need to do this:
I believe it's actually "lower than Op", and since POSTFIXOP is gone
it doesn't seem to matter how low it is. In fact, I found that the
lines with INDENT and UNBOUNDED now work as the lowest precedence
declarations. Maybe that's worth something?
Following on Peter E.'s example upthread, GENERATED can be removed
from precedence, and I also found the same is true for PRESERVE and
STRIP_P.
I've attached a patch which applies on top of 0001 to demonstrate
this. There might possibly still be syntax errors for things not
covered in the regression test, but there are no s/r conflicts at
least.
-{ oid => '389', descr => 'deprecated, use ! instead',
+{ oid => '389', descr => 'factorial',
Hmm, no objection, but it could be argued that we should just go ahead
and remove "!!" also, keeping only "factorial()". If we're going to
break a small amount of code using the normal math expression, it
seems silly to use a non-standard one that we deprecated before 2011
(cf. 908ab802864). On the other hand, removing it doesn't buy us
anything.
Some leftovers...
...in catalog/namespace.c:
OpernameGetOprid()
* Pass oprleft = InvalidOid for a prefix op, oprright = InvalidOid for
* a postfix op.
OpernameGetCandidates()
* The returned items always have two args[] entries --- one or the other
* will be InvalidOid for a prefix or postfix oprkind. nargs is 2, too.
...in nodes/print.c:
/* we print prefix and postfix ops the same... */
> > 0002:
> >
> > + * All keywords can be used explicitly as a column label in expressions
> > + * like 'SELECT 1234 AS keyword', but only some keywords can be used
> > + * implicitly as column labels in expressions like 'SELECT 1234 keyword'.
> > + * Those that can be used implicitly should be listed here.
> >
> > In my mind, "AS" is the thing that's implied when not present, so we
> > should reword this to use the "bare" designation when talking about
> > the labels. I think there are contexts elsewhere where the implicit
> > column label is "col1, col2, col3...". I can't remember offhand where
> > that is though.
>
> Per my rambling above, I think what's really implied or explicit when "AS" is
> missing
Re: factorial function/phase out postfix operators?
> On Aug 24, 2020, at 12:28 AM, John Naylor wrote:
>
> On Tue, May 19, 2020 at 5:03 AM Tom Lane wrote:
>>
>> Peter Eisentraut writes:
>>> What are the thoughts about then marking the postfix operator deprecated
>>> and eventually removing it?
>>
>> If we do this it'd require a plan. We'd have to also warn about the
>> feature deprecation in (at least) the CREATE OPERATOR man page, and
>> we'd have to decide how many release cycles the deprecation notices
>> need to stand for.
>>
>> If that's the intention, though, it'd be good to get those deprecation
>> notices published in v13 not v14.
>
> I imagine the release candidates are not too far away by now, and if
> we are confident enough in the direction the patches in this thread
> are going, we should probably consider a deprecation notice soon.
If so, we might want to also update the deprecation warning for the prefix !!
operator in pg_operator.dat:
{ oid => '389', descr => 'deprecated, use ! instead',
oprname => '!!', oprkind => 'l', oprleft => '0', oprright => 'int8',
oprresult => 'numeric', oprcode => 'numeric_fac' },
That will be the only remaining factorial operator if we remove postfix
operators.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
On Tue, May 19, 2020 at 5:03 AM Tom Lane wrote: > > Peter Eisentraut writes: > > What are the thoughts about then marking the postfix operator deprecated > > and eventually removing it? > > If we do this it'd require a plan. We'd have to also warn about the > feature deprecation in (at least) the CREATE OPERATOR man page, and > we'd have to decide how many release cycles the deprecation notices > need to stand for. > > If that's the intention, though, it'd be good to get those deprecation > notices published in v13 not v14. I imagine the release candidates are not too far away by now, and if we are confident enough in the direction the patches in this thread are going, we should probably consider a deprecation notice soon. -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: factorial function/phase out postfix operators?
On Wed, Jul 22, 2020 at 8:47 AM Mark Dilger
wrote:
>
>
>
> > On Jul 18, 2020, at 1:00 AM, John Naylor
> > wrote:
> >
> > pg_get_keywords() should probably have a column to display ability to
> > act as a bare col label. Perhaps a boolean? If so, what do you think
> > of using true/false for the new field in kwlist.h as well?
Hi Mark, sorry for the delay.
> I have broken this into its own patch. I like using a BARE_LABEL /
> EXPLICIT_LABEL in kwlist.h because it is self-documenting. I don't care
> about the *exact* strings that we choose for that, but using TRUE/FALSE in
> kwlist.h makes it harder for a person adding a new keyword to know what to
> place there. If they guess "FALSE", and also don't know about adding the new
> keyword to the bare_label_keyword rule in gram.y, then those two mistakes
> will agree with each other and the person adding the keyword won't likely
> know they did it wrong. It is simple enough for gen_keywordlist.pl to
> convert between what we use in kwlist.h and a boolean value for kwlist_d.h,
> so I did it that way.
Sounds fine to me.
> > In the bikeshedding department, it seems "implicit" was chosen because
> > it was distinct from "bare". I think "bare" as a descriptor should be
> > kept throughout for readability's sake. Maybe BareColLabel could be
> > "IDENT or bare_label_keyword" for example. Same for the $status var.
>
> The category "bare_label_keyword" is used in v3. As for the $status var, I
> don't want to name that $bare, as I didn't go with your idea about using a
> boolean. $status = "BARE_LABEL" vs "EXPLICIT_LABEL" makes sense to me, more
> than $bare = "BARE_LABEL" vs "EXPLICIT_LABEL" does. "status" is still a bit
> vague, so more bikeshedding is welcome.
Yeah, it's very generic, but it's hard to find a short word for
"can-be-used-as-a-bare-column-label-ness".
> This patch does not attempt to remove pre-existing postfix operators from
> existing databases, so users upgrading to the new major version who have
> custom postfix operators will find that pg_upgrade chokes trying to recreate
> the postfix operator. That's not great, but perhaps there is nothing
> automated that we could do for them that would be any better.
I'm thinking it would be good to have something like
select oid from pg_operator where oprright = 0 and oid >= FirstNormalObjectId;
in the pre-upgrade check.
Other comments:
0001:
+ errhint("postfix operator support has been discontinued")));
This language seems more appropriate for release notes -- I would word
the hint in the present, as in "postfix operators are not supported".
Ditto the words "discontinuation", "has been removed", and "no longer
works" elsewhere in the patch.
+SELECT -5!;
+SELECT -0!;
+SELECT 0!;
+SELECT 100!;
I think one negative and one non-negative case is enough to confirm
the syntax error.
- gram.y still contains "POSTFIXOP" and "postfix-operator".
- parse_expr.c looks like it has some now-unreachable code.
0002:
+ * All keywords can be used explicitly as a column label in expressions
+ * like 'SELECT 1234 AS keyword', but only some keywords can be used
+ * implicitly as column labels in expressions like 'SELECT 1234 keyword'.
+ * Those that can be used implicitly should be listed here.
In my mind, "AS" is the thing that's implied when not present, so we
should reword this to use the "bare" designation when talking about
the labels. I think there are contexts elsewhere where the implicit
column label is "col1, col2, col3...". I can't remember offhand where
that is though.
- * kwlist.h's table from one source of truth.)
+ * kwlist.h's table from a common master list.)
Off topic.
0003:
First off, I get a crash when trying
select * from pg_get_keywords();
and haven't investigated further. I don't think the returned types
match, though.
Continuing on, I think 2 and 3 can be squashed together. If anything,
it should make revisiting cosmetic decisions easier.
+ TupleDescInitEntry(tupdesc, (AttrNumber) 4, "bare",
+BOOLOID, -1, 0);
Perhaps something a bit meatier for the user-visible field name. I
don't have a great suggestion.
- proname => 'pg_get_keywords', procost => '10', prorows => '400',
+ proname => 'pg_get_keywords', procost => '10', prorows => '450',
Off topic for this patch. Not sure it matters much, either.
"EXPLICIT_LABEL" -- continuing my line of thought above, all labels
are explicit, that's why they're called labels. Brainstorm:
EXPLICIT_AS_LABEL
EXPLICIT_AS
NON_BARE_LABEL
*shrug*
+ # parser/kwlist.h lists each keyword as either bare or
+ # explicit, but ecpg neither needs nor has any such
PL/pgSQL also uses this script, so maybe just phrase it to exclude the
core keyword list.
--
John Naylorhttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: factorial function/phase out postfix operators?
On Sat, Jul 11, 2020 at 1:14 AM Mark Dilger wrote: > > > Tom and Álvaro discussed upthread: > > > >> Would it make sense (and possible) to have a keyword category that is > >> not disjoint wrt. the others? Maybe that ends up being easier than > >> a solution that ends up with six or seven categories. > > Version 2, attached, follows this design, increasing the number of keywords > that can be used as column aliases without the AS keyword up to 411, with > only 39 keywords still requiring an explicit preceding AS. Hi Mark, This isn't a full review, but I have a few questions/comments: By making col-label-ness an orthogonal attribute, do we still need the category of non_label_keyword? It seems not. pg_get_keywords() should probably have a column to display ability to act as a bare col label. Perhaps a boolean? If so, what do you think of using true/false for the new field in kwlist.h as well? In the bikeshedding department, it seems "implicit" was chosen because it was distinct from "bare". I think "bare" as a descriptor should be kept throughout for readability's sake. Maybe BareColLabel could be "IDENT or bare_label_keyword" for example. Same for the $status var. Likewise, it seems the actual removal of postfix operators should be a separate patch. -- John Naylorhttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: factorial function/phase out postfix operators?
> On Jun 30, 2020, at 2:47 PM, Mark Dilger wrote: > > > >> On May 19, 2020, at 4:47 PM, Tom Lane wrote: >> >> I wrote: >>> However, we do have to have a benefit to show those people whose >>> queries we break. Hence my insistence on having a working AS fix >>> (or some other benefit) before not after. >> >> I experimented with this a bit more, and came up with the attached. >> It's not a working patch, just a set of grammar changes that Bison >> is happy with. (Getting to a working patch would require fixing the >> various build infrastructure that knows about the keyword classification, >> which seems straightforward but tedious.) > > I built a patch on top of yours that does much of that tedious work. > >> As Robert theorized, it works to move a fairly-small number of unreserved >> keywords into a new slightly-reserved category. However, as the patch >> stands, only the remaining fully-unreserved keywords can be used as bare >> column labels. I'd hoped to be able to also use col_name keywords in that >> way (which'd make the set of legal bare column labels mostly the same as >> ColId). The col_name keywords that cause problems are, it appears, >> only PRECISION, CHARACTER, and CHAR_P. So in principle we could move >> those three into yet another keyword category and then let the remaining >> col_name keywords be included in BareColLabel. I kind of think that >> that's more complication than it's worth, though. > > By my count, 288 more keywords can be used as column aliases without the AS > keyword after the patch. That exactly matches what Robert said upthread. > > Tom and Álvaro discussed upthread: > >> Would it make sense (and possible) to have a keyword category that is >> not disjoint wrt. the others? Maybe that ends up being easier than >> a solution that ends up with six or seven categories. Version 2, attached, follows this design, increasing the number of keywords that can be used as column aliases without the AS keyword up to 411, with only 39 keywords still requiring an explicit preceding AS. v2-0001-Allow-most-keywords-to-be-used-as-implicit-column.patch Description: Binary data — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
Peter Eisentraut writes: > What I was hoping to get out of this was to resolve some of the weird > precedence hacks that were blamed on postfix operators. Yeah, I was thinking about that too, but hadn't gotten to it. > But building on your patch, the best I could achieve was > -%nonassoc IDENT GENERATED NULL_P PARTITION RANGE ROWS GROUPS PRECEDING > FOLLOWING CUBE ROLLUP > +%nonassoc IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE > ROLLUP > which is a pretty poor yield. I'd hoped for better as well. Still, it's possible this would save us from greater pain in the future, seeing that the SQL committee seems resolutely uninterested in whether the syntax they invent is parsable. (Also, there are other factors here: I think at least some of those precedence hacks are there to avoid fully reserving the associated keywords.) > Maybe this isn't worth it after all. It'd be nice to have a better yield from removing a user-visible feature. Perhaps there would be no complaints about removing postfix ops, but if there are I want to be able to point to some substantial benefit that users get from it. (Which is why I focused on the optional-AS business to start with ... users don't care about how many precedence hacks we need.) regards, tom lane
Re: factorial function/phase out postfix operators?
On 2020-05-20 01:47, Tom Lane wrote: I wrote: However, we do have to have a benefit to show those people whose queries we break. Hence my insistence on having a working AS fix (or some other benefit) before not after. I experimented with this a bit more, and came up with the attached. It's not a working patch, just a set of grammar changes that Bison is happy with. (Getting to a working patch would require fixing the various build infrastructure that knows about the keyword classification, which seems straightforward but tedious.) What I was hoping to get out of this was to resolve some of the weird precedence hacks that were blamed on postfix operators. But building on your patch, the best I could achieve was -%nonassoc IDENT GENERATED NULL_P PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP +%nonassoc IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP which is a pretty poor yield. Maybe this isn't worth it after all. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: factorial function/phase out postfix operators?
On Wed, May 20, 2020 at 2:24 PM Tom Lane wrote: > Right; I'd done the same arithmetic. Since we currently have a total > of 450 keywords of all flavors, that means we can make either 64% > of them or 74.6% of them be safe to use as bare column labels. While > that's surely better than today, it doesn't seem like it's going to > make for any sort of sea change in the extent of the problem. So I was > feeling a bit discouraged by these results. I don't think you should feel discouraged by these results. They assume that people are just as likely to have a problem with a reserved keyword as an unreserved keyword, and I don't think that's actually true. The 25.4% of keywords that aren't handled this way include, to take a particularly egregious example, "AS" itself. And I don't think many people are going to be sad if "select 1 as;" fails to treat "as" as a column label. Also, even if we only made 74.6% of these safe to use as bare column labels, or even 64%, I think that's actually pretty significant. If I could reduce my mortgage payment by 64%, I would be pretty happy. For many people, that would be a sufficiently large economic impact that it actually would be a sea change in terms of their quality of life. I don't see a reason to suppose that's not also true here.[1] I do like the idea of considering "can be a bare column label" as an independent dimension from the existing keyword classification. Presumably we would then have, in addition to the four existing keyword productions, but then also a separate bare_column_label_keyword: production that would include many of the same keywords. One nice thing about that approach is that we would then have a clear list of exactly which keywords can't be given that treatment, and if somebody wanted to go investigate possible improvements for any of those, they could do so. I think we'd want a cross-check: check_keywords.pl should contain the list of keywords that are expected to be excluded from this new production, so that any time someone adds a new keyword, they've either got to add it to the new production or add it to the exception list. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company [1] On the other hand, if I had 64% fewer ants in my picnic basket, I would probably still be unhappy with the number of ants in my picnic basket, so it all depends on context and perspective.
Re: factorial function/phase out postfix operators?
Alvaro Herrera writes: > On 2020-May-20, Tom Lane wrote: >> I feel like we'd be better advised to somehow >> treat can-be-bare-col-label as an independent classification. > Would it make sense (and possible) to have a keyword category that is > not disjoint wrt. the others? Maybe that ends up being easier than > a solution that ends up with six or seven categories. Yeah, that's the same thing I was vaguely imagining -- an independent flag on each keyword as to whether it can be used as a bare column alias. regards, tom lane
Re: factorial function/phase out postfix operators?
On 2020-May-20, Tom Lane wrote: > I too failed to save the results of some experimentation, but I'd > also poked at the type_func_name_keyword category, and it has a similar > situation where only about three keywords cause problems if included > in BareColLabel. So we could possibly get another twenty-ish keywords > into that set with yet a third new keyword category. But (a) we'd still > only be at 79% coverage and (b) this is *really* making things messy > keyword-category-wise. I feel like we'd be better advised to somehow > treat can-be-bare-col-label as an independent classification. > > (I did not look at whether any of the fully-reserved keywords could > be made safe to use, but it seems likely that at least some of them > could be, if we accept even more classification mess.) Would it make sense (and possible) to have a keyword category that is not disjoint wrt. the others? Maybe that ends up being easier than a solution that ends up with six or seven categories. -- Álvaro Herrerahttps://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: factorial function/phase out postfix operators?
Mark Dilger writes: > ... But if we made a clean distinction between the characters that are > allowed in postfix operators vs. those allowed for infix operators, then we'd > get to have postfix operators without the ambiguity, right? I continue to see little point in half-baked compatibility measures like that. You'd be much more likely to break working setups (that might not even involve any postfix operators) than to accomplish anything useful. In particular, if Joe DBA out there has a postfix operator, and it's not named according to whatever rule you chose, then you haven't done anything to fix his compatibility problem. > When thinking about postfix operators, the subscript and superscript > character ranges come to my mind, such as > SELECT Σ₂(x² + y³ + z⁴); We already have a convention about non-ASCII characters, and it is that they are identifier characters not operator characters. Changing that would break yet a different set of applications. (That is to say, the above SELECT already has a well-defined lexical interpretation.) regards, tom lane
Re: factorial function/phase out postfix operators?
> On May 20, 2020, at 11:24 AM, Tom Lane wrote: > > Bottom line is that we can reduce the scope of the col-label problem > this way, but we can't make it go away entirely. Is a partial solution > to that worth a full drop of postfix operators? Possibly, but I'm not > sure. I still feel like it'd be worth investigating some other solution > technology, ie lookahead, though I concede your point that that has > pitfalls too. I should think a lot of the problem stems from allowing the same characters to be used in postfix operators as in other operators. The ! character is already not allowed as a column alias: +SELECT 1 AS ! ORDER BY !; +ERROR: syntax error at or near "!" +LINE 1: SELECT 1 AS ! ORDER BY !; +^ But you can use it as a prefix or infix operator, which creates the confusion about whether SELECT 5 ! x Means "x" as an alias or as the right argument to the ! infix operator. But if we made a clean distinction between the characters that are allowed in postfix operators vs. those allowed for infix operators, then we'd get to have postfix operators without the ambiguity, right? When thinking about postfix operators, the subscript and superscript character ranges come to my mind, such as SELECT Σ₂(x² + y³ + z⁴); These also come to mind as prefix operators, but I don't recall seeing them as infix operators, so maybe it would be ok to disallow that? As for the ! infix operator, it doesn't exist by default: +SELECT x ! y from (select 5 AS x, 3 AS y) AS ss; +ERROR: operator does not exist: integer ! integer +LINE 1: SELECT x ! y from (select 5 AS x, 3 AS y) AS ss; + ^ +HINT: No operator matches the given name and argument types. You might need to add explicit type casts. So if we put that in the set of characters disallowed for infix operators, we would only be breaking custom infix operators named that, which seems like less breakage to me than removing postfix operators of all kinds. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
Robert Haas writes: > On Tue, May 19, 2020 at 7:47 PM Tom Lane wrote: >> However, as the patch >> stands, only the remaining fully-unreserved keywords can be used as bare >> column labels. I'd hoped to be able to also use col_name keywords in that >> way (which'd make the set of legal bare column labels mostly the same as >> ColId). The col_name keywords that cause problems are, it appears, >> only PRECISION, CHARACTER, and CHAR_P. So in principle we could move >> those three into yet another keyword category and then let the remaining >> col_name keywords be included in BareColLabel. I kind of think that >> that's more complication than it's worth, though. > I think it's a judgement call. If all we do is what you have in the > patch, we can make 288 keywords that currently aren't usable as column > labels without AS, plus future unreserved keywords that get similar > treatment. If we also split the column-name keywords, then we can buy > ourselves another 48 keywords that can be used as column labels > without AS. Presumably everybody is going to agree that allowing more > keywords to be used this way is better than fewer, but also that > having fewer keyword classifications is better than having more, and > those goals are in tension in this case. Right; I'd done the same arithmetic. Since we currently have a total of 450 keywords of all flavors, that means we can make either 64% of them or 74.6% of them be safe to use as bare column labels. While that's surely better than today, it doesn't seem like it's going to make for any sort of sea change in the extent of the problem. So I was feeling a bit discouraged by these results. I too failed to save the results of some experimentation, but I'd also poked at the type_func_name_keyword category, and it has a similar situation where only about three keywords cause problems if included in BareColLabel. So we could possibly get another twenty-ish keywords into that set with yet a third new keyword category. But (a) we'd still only be at 79% coverage and (b) this is *really* making things messy keyword-category-wise. I feel like we'd be better advised to somehow treat can-be-bare-col-label as an independent classification. (I did not look at whether any of the fully-reserved keywords could be made safe to use, but it seems likely that at least some of them could be, if we accept even more classification mess.) Bottom line is that we can reduce the scope of the col-label problem this way, but we can't make it go away entirely. Is a partial solution to that worth a full drop of postfix operators? Possibly, but I'm not sure. I still feel like it'd be worth investigating some other solution technology, ie lookahead, though I concede your point that that has pitfalls too. regards, tom lane
Re: factorial function/phase out postfix operators?
On Tue, May 19, 2020 at 7:47 PM Tom Lane wrote: > As Robert theorized, it works to move a fairly-small number of unreserved > keywords into a new slightly-reserved category. It wasn't entirely a theoretical argument, since I'm pretty sure I did spend some time experimenting with gram.y back in the day, but possibly not to the extent that you've done here. And I seem not to have saved my work, either... > However, as the patch > stands, only the remaining fully-unreserved keywords can be used as bare > column labels. I'd hoped to be able to also use col_name keywords in that > way (which'd make the set of legal bare column labels mostly the same as > ColId). The col_name keywords that cause problems are, it appears, > only PRECISION, CHARACTER, and CHAR_P. So in principle we could move > those three into yet another keyword category and then let the remaining > col_name keywords be included in BareColLabel. I kind of think that > that's more complication than it's worth, though. I think it's a judgement call. If all we do is what you have in the patch, we can make 288 keywords that currently aren't usable as column labels without AS, plus future unreserved keywords that get similar treatment. If we also split the column-name keywords, then we can buy ourselves another 48 keywords that can be used as column labels without AS. Presumably everybody is going to agree that allowing more keywords to be used this way is better than fewer, but also that having fewer keyword classifications is better than having more, and those goals are in tension in this case. I believe that most, possibly all, of the examples of this problem that I have seen involve unreserved keywords, but that might just because there are a lot more unreserved keywords than there are keywords of any other sort. Things like TIME, POSITION, and VALUES don't seem like particularly unlikely choices for a column label. I mean, someone who knows SQL well and is a good programmer might not choose these things, either because they're kind of generic, or because they're known to have special meaning in SQL. However, SQL is used by many people who don't know it well and aren't good programmers, and people coming from other database systems generally don't have to worry much about their choice of column labels and then get sad when their migration fails. So I'd be somewhat inclined to see how far we can reasonably push this, but I'm also entirely willing to accept that 85% of a loaf is better than none. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
I wrote:
> However, we do have to have a benefit to show those people whose
> queries we break. Hence my insistence on having a working AS fix
> (or some other benefit) before not after.
I experimented with this a bit more, and came up with the attached.
It's not a working patch, just a set of grammar changes that Bison
is happy with. (Getting to a working patch would require fixing the
various build infrastructure that knows about the keyword classification,
which seems straightforward but tedious.)
As Robert theorized, it works to move a fairly-small number of unreserved
keywords into a new slightly-reserved category. However, as the patch
stands, only the remaining fully-unreserved keywords can be used as bare
column labels. I'd hoped to be able to also use col_name keywords in that
way (which'd make the set of legal bare column labels mostly the same as
ColId). The col_name keywords that cause problems are, it appears,
only PRECISION, CHARACTER, and CHAR_P. So in principle we could move
those three into yet another keyword category and then let the remaining
col_name keywords be included in BareColLabel. I kind of think that
that's more complication than it's worth, though.
regards, tom lane
diff --git a/src/backend/parser/gram.y b/src/backend/parser/gram.y
index a24b30f..0b034b6 100644
--- a/src/backend/parser/gram.y
+++ b/src/backend/parser/gram.y
@@ -542,13 +542,14 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%type Sconst comment_text notify_payload
%type RoleId opt_boolean_or_string
%type var_list
-%type ColId ColLabel var_name type_function_name param_name
+%type ColId ColLabel BareColLabel
%type NonReservedWord NonReservedWord_or_Sconst
+%type var_name type_function_name param_name
%type createdb_opt_name
%type var_value zone_value
%type auth_ident RoleSpec opt_granted_by
-%type unreserved_keyword type_func_name_keyword
+%type unreserved_keyword non_label_keyword type_func_name_keyword
%type col_name_keyword reserved_keyword
%type TableConstraint TableLikeClause
@@ -744,7 +745,6 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
%nonassoc '<' '>' '=' LESS_EQUALS GREATER_EQUALS NOT_EQUALS
%nonassoc BETWEEN IN_P LIKE ILIKE SIMILAR NOT_LA
%nonassoc ESCAPE /* ESCAPE must be just above LIKE/ILIKE/SIMILAR */
-%left POSTFIXOP /* dummy for postfix Op rules */
/*
* To support target_el without AS, we must give IDENT an explicit priority
* between POSTFIXOP and Op. We can safely assign the same priority to
@@ -3908,6 +3908,7 @@ PartitionSpec: PARTITION BY part_strategy '(' part_params ')'
part_strategy: IDENT { $$ = $1; }
| unreserved_keyword { $$ = pstrdup($1); }
+| non_label_keyword { $$ = pstrdup($1); }
;
part_params: part_elem { $$ = list_make1($1); }
@@ -13230,8 +13231,6 @@ a_expr: c_expr { $$ = $1; }
{ $$ = (Node *) makeA_Expr(AEXPR_OP, $2, $1, $3, @2); }
| qual_Op a_expr %prec Op
{ $$ = (Node *) makeA_Expr(AEXPR_OP, $1, NULL, $2, @1); }
- | a_expr qual_Op %prec POSTFIXOP
-{ $$ = (Node *) makeA_Expr(AEXPR_OP, $2, $1, NULL, @2); }
| a_expr AND a_expr
{ $$ = makeAndExpr($1, $3, @2); }
@@ -13645,8 +13644,6 @@ b_expr: c_expr
{ $$ = (Node *) makeA_Expr(AEXPR_OP, $2, $1, $3, @2); }
| qual_Op b_expr %prec Op
{ $$ = (Node *) makeA_Expr(AEXPR_OP, $1, NULL, $2, @1); }
- | b_expr qual_Op %prec POSTFIXOP
-{ $$ = (Node *) makeA_Expr(AEXPR_OP, $2, $1, NULL, @2); }
| b_expr IS DISTINCT FROM b_expr %prec IS
{
$$ = (Node *) makeSimpleA_Expr(AEXPR_DISTINCT, "=", $1, $5, @2);
@@ -14910,7 +14907,7 @@ target_el: a_expr AS ColLabel
* as an infix expression, which we accomplish by assigning
* IDENT a precedence higher than POSTFIXOP.
*/
- | a_expr IDENT
+ | a_expr BareColLabel
{
$$ = makeNode(ResTarget);
$$->name = $2;
@@ -15228,6 +15225,7 @@ role_list: RoleSpec
*/
ColId: IDENT { $$ = $1; }
| unreserved_keyword { $$ = pstrdup($1); }
+ | non_label_keyword { $$ = pstrdup($1); }
| col_name_keyword { $$ = pstrdup($1); }
;
@@ -15235,6 +15233,7 @@ ColId: IDENT { $$ = $1; }
*/
type_function_name: IDENT { $$ = $1; }
| unreserved_keyword { $$ = pstrdup($1); }
+ | non_label_keyword { $$ = pstrdup($1); }
| type_func_name_keyword{ $$ = pstrdup($1); }
;
@@ -15242,15 +15241,23 @@ type_function_name: IDENT { $$ = $1; }
*/
NonReservedWord: IDENT { $$ = $1; }
| unreserved_keyword { $$ = pstrdup($1); }
+ | non_label_keyword { $$ = pstrdup($1); }
| col_name_keyword { $$ = pstrdup($1); }
| type_func_name_keyword{ $$ = pstrdup($1); }
;
+/* Bare column label --- names that can be column labels without writing "AS".
+ */
+BareColLabel: IDENT{ $$ = $1; }
+ | unreserved_keyword { $$ =
Re: factorial function/phase out postfix operators?
Robert Haas writes: > On Tue, May 19, 2020 at 2:30 PM Tom Lane wrote: >> Anyway, the bottom-line conclusion remains the same: let's make sure >> we know what we'd do after getting rid of postfix ops, before we do >> that. > Well, I don't think we really need to get too conservative here. > ... It seems to me that > the first thing that we need to do here is get a deprecation notice > out, so that people know that we're planning to break this. No, I disagree with that, because from what I've seen so far it's not really clear to me that we have a full solution to the AS problem excepting only postfix ops. I don't want to deprecate postfix ops before it's completely clear that we can get something out of it. Otherwise, we'll either end up un-deprecating them, which makes us look silly, or removing a feature for zero benefit. Stephen's nearby proposal to deprecate only after a patch has been committed doesn't seem all that unreasonable, if you're only intending to allow one cycle's worth of notice. In particular, I could easily see us committing a fix sometime this summer and then sticking deprecation notices into the back branches before v13 goes gold. But let's have the complete fix in hand first. > I'm still interested in hearing what people think about hard-coding ! > as a postfix operator vs. removing postfix operators altogether. I > think Vik and Tom are against keeping just !, Kenneth Marshall are for > it, and I'm not sure I understand Pavel's position. Yes, I'm VERY strongly against keeping just !. I think it'd be a ridiculous, and probably very messy, backwards-compatibility hack; and the fact that it will break valid use-cases that we don't need to break seems to me to well outweigh the possibility that someone would rather not change their queries to use factorial() or !!. However, we do have to have a benefit to show those people whose queries we break. Hence my insistence on having a working AS fix (or some other benefit) before not after. regards, tom lane
Re: factorial function/phase out postfix operators?
On Tue, May 19, 2020 at 2:30 PM Tom Lane wrote: > Might work. My main concern would be if we have to forbid those keywords > as column names --- for words like "year", in particular, that'd be a > disaster. If the net effect is only that they can't be AS-less col labels, > it won't break any cases that worked before. ISTM that all we have to do to avoid that is switch from a four-way classification to a five-way classification: just split unreserved_keyword into totally_unreserved_keyword and very_slightly_reserved_keyword. > Our existing four-way keyword classification is not something that was > handed down on stone tablets. I wonder whether postfix-ectomy changes > the situation enough that a complete rethinking would be helpful. I don't see that they do, but I might be missing something. I think there's an excellent argument for adding one new category, but it's not clear to me why it should reshape the landscape any more than that. > I also continue to think that more lookahead and token-merging would > be interesting to pursue. It'd hardly surprise anybody if the > token pair "character varying" were always treated as a type name, > for instance. I think that line of attack will not buy very much. The ability to avoid unexpected consequences is entirely contingent on the unlikeliness of the keywords appearing adjacent to each other in some other context, and the only argument for that here is that neither of those words is a terribly likely column name. I think that when you try to solve interesting problems with this, though, you very quickly run into problems where that's not the case, and you'll need a technique that has some knowledge of the parser state to actually do something that works well. I read a paper some years ago that proposed a solution to this problem: if the parser generator sees a shift/reduce conflict, it checks whether the conflict can be resolve by looking ahead one or more additional tokens. If so, it can build a little DFA that gets run when you enter that state, with edges labeled with lookahead tokens, and it runs that DFA whenever you reach the problematic state. Since, hopefully, such states are relatively rarely encountered, the overhead is low, yet it still gives you a way out of conflicts in many practical cases. Unfortunately, the chances of bison implementing such a thing do not seem very good. > Anyway, the bottom-line conclusion remains the same: let's make sure > we know what we'd do after getting rid of postfix ops, before we do > that. Well, I don't think we really need to get too conservative here. I've studied this issue enough over the years to be pretty darn sure that this is a necessary prerequisite to doing something about the "AS unreserved_keyword" issue, and that it is by far the most significant issue in doing something about that problem. Sure, there are other issues, but I think they are basically matters of politics or policy. For example, if some key people DID think that the four-way keyword classification was handed down on stone tablets, that could be quite a problem, but if we're willing to take the view that solving the "AS unreserved_keyword" problem is pretty important and we need to find a way to get it done, then I think we an do that. It seems to me that the first thing that we need to do here is get a deprecation notice out, so that people know that we're planning to break this. I think we should go ahead and make that happen now, or at least pretty soon. I'm still interested in hearing what people think about hard-coding ! as a postfix operator vs. removing postfix operators altogether. I think Vik and Tom are against keeping just !, Kenneth Marshall are for it, and I'm not sure I understand Pavel's position. I'm about +0.3 for keeping just ! myself. Maybe we'll get some other votes. If you're willing to be persuaded that keeping only ! is a sensible thing to consider, I could probably draft a very rough patch showing that it would still be sufficient to get us out from under the "AS unreserved_keyword" problem, but if you and/or enough other people hate that direction with a fiery passion, I won't bother. I'm pretty sure it's technically possible, but the issue is more about what people actually want. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
Robert Haas writes: > On Tue, May 19, 2020 at 11:32 AM Tom Lane wrote: >> Before we go much further on this, we should have some proof >> that there's actually material benefit to be gained. I spent some >> time just now trying to relax the AS restriction by ripping out >> postfix ops, and the results were not too promising. > I came to similar conclusions a couple of years ago: > https://www.postgresql.org/message-id/ca+tgmoyzpvt7uihjwgktytivhhlncp0ylavcoipe-lyg3w2...@mail.gmail.com Ah, right. > What I proposed at the time was creating a new category of keywords. Might work. My main concern would be if we have to forbid those keywords as column names --- for words like "year", in particular, that'd be a disaster. If the net effect is only that they can't be AS-less col labels, it won't break any cases that worked before. Our existing four-way keyword classification is not something that was handed down on stone tablets. I wonder whether postfix-ectomy changes the situation enough that a complete rethinking would be helpful. I also continue to think that more lookahead and token-merging would be interesting to pursue. It'd hardly surprise anybody if the token pair "character varying" were always treated as a type name, for instance. Anyway, the bottom-line conclusion remains the same: let's make sure we know what we'd do after getting rid of postfix ops, before we do that. regards, tom lane
Re: factorial function/phase out postfix operators?
On Tue, May 19, 2020 at 11:32 AM Tom Lane wrote: > Before we go much further on this, we should have some proof > that there's actually material benefit to be gained. I spent some > time just now trying to relax the AS restriction by ripping out > postfix ops, and the results were not too promising. Indeed the > postfix-ops problem goes away, but then you find out that SQL's > random syntax choices for type names become the stumbling block. > An example here is that given > > SELECT 'foo'::character varying > > it's not clear if "varying" is supposed to be part of the type name or a > column label. It looks to me like we'd have to increase the reserved-ness > of VARYING, PRECISION, and about half a dozen currently-unreserved > keywords involved in INTERVAL syntax, including such popular column names > as "month", "day", and "year". > > Plus I got conflicts on WITHIN, GROUP, and FILTER from ordered-set > aggregate syntax; those are currently unreserved keywords, but they > can't be allowed as AS-less column labels. I came to similar conclusions a couple of years ago: https://www.postgresql.org/message-id/ca+tgmoyzpvt7uihjwgktytivhhlncp0ylavcoipe-lyg3w2...@mail.gmail.com What I proposed at the time was creating a new category of keywords. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
Vik Fearing writes: > I'm -1 on keeping ! around as a hard-coded postfix operator. Before we go much further on this, we should have some proof that there's actually material benefit to be gained. I spent some time just now trying to relax the AS restriction by ripping out postfix ops, and the results were not too promising. Indeed the postfix-ops problem goes away, but then you find out that SQL's random syntax choices for type names become the stumbling block. An example here is that given SELECT 'foo'::character varying it's not clear if "varying" is supposed to be part of the type name or a column label. It looks to me like we'd have to increase the reserved-ness of VARYING, PRECISION, and about half a dozen currently-unreserved keywords involved in INTERVAL syntax, including such popular column names as "month", "day", and "year". Plus I got conflicts on WITHIN, GROUP, and FILTER from ordered-set aggregate syntax; those are currently unreserved keywords, but they can't be allowed as AS-less column labels. We could possibly minimize the damage by inventing another keyword classification besides the four we have now. Or maybe we should think harder about using more lookahead between the lexer and grammar. But this is going to be a lot more ticklish than I would've hoped, and possibly not cost-free, so we might well end up never pulling the trigger on such a change. So right at the moment I'm agreeing with Stephen's nearby opinion: let's not deprecate these until we've got a patch that gets some concrete benefit from removing them. regards, tom lane
Re: factorial function/phase out postfix operators?
Greetings, * Vik Fearing (v...@postgresfriends.org) wrote: > On 5/19/20 4:03 AM, Tom Lane wrote: > > Peter Eisentraut writes: > >> What are the thoughts about then marking the postfix operator deprecated > >> and eventually removing it? > > > > If we do this it'd require a plan. We'd have to also warn about the > > feature deprecation in (at least) the CREATE OPERATOR man page, and > > we'd have to decide how many release cycles the deprecation notices > > need to stand for. > > I have never come across any custom postfix operators in the wild, and > I've never even seen ! used in practice. > > So I would suggest a very short deprecation period. Deprecate now in > 13, let 14 go by, and rip it all out for 15. That should give us enough > time to extend the deprecation period if we need to, or go back on it > entirely (like I seem to remember we did with VACUUM FREEZE). > > > If that's the intention, though, it'd be good to get those deprecation > > notices published in v13 not v14. > > +1 I agree with putting notices into v13 saying they're deprecated, but then actually removing them in v14. For that matter, I'd vote that we generally accept a system whereby when we commit something that removes a feature in the next major version, we put out some kind of notice that it's been deprecated and won't be in v14. We don't want to run the risk of saying XYZ has been deprecated and then it staying around for a few years, nor trying to say "it'll be removed in v14" before we actually know that it's been committed for v14. In other words, wait to deprecate until the commit has happened for v14 (and maybe wait a couple days in case someone wasn't watching and argues to revert, but not longer than any normal commit), and then go back and mark it as "deprecated and removed in v14" for all back-branches. Users will continue to have 5 years (by upgrading to v13, or whatever the last release was before their favorite feature was removed, if they really need to) to update their systems to deal with the change. We do not do ourselves nor our users a real service by carrying forward deprecated code/interfaces/views/etc, across major versions; instead they tend to live on in infamy, with some users actually updating and some not, ever, and then complaining when we suggest actually removing it (we have lots of good examples of that too) and then we have to have the debate again about removing it and, in some cases, we end up un-deprecating it, which is confusing for users and a bit ridiculous. Let's not do that. Thanks, Stephen signature.asc Description: PGP signature
Re: factorial function/phase out postfix operators?
Robert Haas writes: > The ambiguity doesn't come from the mere existence of postfix > operators. It comes from the fact that, when we lex the input, we > can't tell whether a particular operator that we happen to encounter > is prefix, infix, or postfix. So hard-coding, for example, a rule that > '!' is always a postfix operator and anything else is never a postfix > operator is sufficient to solve the key problems. If we were willing to say that '!' could *only* be a postfix operator, then maybe the ambiguity would go away. Or maybe it wouldn't; if you're seriously proposing this, I think it'd be incumbent on you to demonstrate that we could still simplify the grammar to the same extent. But that will incur its own set of compatibility problems, because there's no reason to assume that nobody has made prefix or infix '!' operators. In any case, it's hard to decide that that's a less klugy solution than getting rid of postfix ops altogether. There's a reason why few programming languages have those. In general, I put this on about the same level as when we decided to remove ';' and ':' as operators (cf 259489bab, 766fb7f70). Somebody thought it was cute that it was possible to have that, which maybe it was, but it wasn't really sane in the big picture. And as I recall, the amount of pushback we got was nil. regards, tom lane
Re: factorial function/phase out postfix operators?
On 5/19/20 4:22 PM, Robert Haas wrote: > On Tue, May 19, 2020 at 9:51 AM Tom Lane wrote: >> Uh ... what exactly would be the point of that? The real reason to do >> this at all is not that we have it in for '!', but that we want to >> drop the possibility of postfix operators from the grammar altogether, >> which will remove a boatload of ambiguity. > > The ambiguity doesn't come from the mere existence of postfix > operators. It comes from the fact that, when we lex the input, we > can't tell whether a particular operator that we happen to encounter > is prefix, infix, or postfix. So hard-coding, for example, a rule that > '!' is always a postfix operator and anything else is never a postfix > operator is sufficient to solve the key problems. Then "SELECT a ! b" > can only be a postfix operator application followed by a column > labeling, a "SELECT a + b" can only be the application of an infix > operator. So if I make a complex UDT where a NOT operator makes a lot of sense[*], why wouldn't I be allowed to make a prefix operator ! for it? All for what? That one person in the corner over there who doesn't want to rewrite their query to use factorial() instead? I'm -1 on keeping ! around as a hard-coded postfix operator. [*] I don't have a concrete example in mind, just this abstract one. -- Vik Fearing
Re: factorial function/phase out postfix operators?
On Tue, May 19, 2020 at 9:51 AM Tom Lane wrote:
> Uh ... what exactly would be the point of that? The real reason to do
> this at all is not that we have it in for '!', but that we want to
> drop the possibility of postfix operators from the grammar altogether,
> which will remove a boatload of ambiguity.
The ambiguity doesn't come from the mere existence of postfix
operators. It comes from the fact that, when we lex the input, we
can't tell whether a particular operator that we happen to encounter
is prefix, infix, or postfix. So hard-coding, for example, a rule that
'!' is always a postfix operator and anything else is never a postfix
operator is sufficient to solve the key problems. Then "SELECT a ! b"
can only be a postfix operator application followed by a column
labeling, a "SELECT a + b" can only be the application of an infix
operator.
The parser ambiguities could also be removed if the source of the
information where a GUC or a catalog lookup; there are good reasons
not to go that way, but my point is that the problem is not that
postfix operators are per se evil, but that the information we need is
not available at the right phase of the process. We can only make use
of the information in pg_operator after we start assigning type
information, which has to happen after we parse, but to avoid the
ambiguity here, we need the information before we parse - i.e. at the
lexing stage.
> In my non-caffeinated state, I don't recall exactly which things are
> blocked by the existence of postfix ops; but I think for instance it might
> become possible to remove the restriction of requiring AS before column
> aliases that happen to be unreserved keywords.
Right - which would be a huge win.
> I would also argue that having a feature that is available to
> built-in operators but not user-defined ones is pretty antithetical
> to Postgres philosophy.
That I think is the policy question before us. I believe that any rule
that tells us which operators are postfix and which are not at the
lexing stage is good enough. I think here you are arguing for the
empty set, which will work, but I believe any other fixed set also
works, such as { '!' }. I don't think we're going to break a ton of
user code no matter which one we pick, but I do think that it's
possible to pick either one and still achieve our goals here, so
that's the issue that I wanted to raise.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
Robert Haas writes: > I think it's generally a good idea, though perhaps we should consider > continuing to allow '!' as a postfix operator and just removing > support for any other. Uh ... what exactly would be the point of that? The real reason to do this at all is not that we have it in for '!', but that we want to drop the possibility of postfix operators from the grammar altogether, which will remove a boatload of ambiguity. > I won't lose a lot of sleep if we decide to rip out '!' as well, but I > don't think that continuing to support it would cost us much. AFAICS, it would cost us the entire point of this change. In my non-caffeinated state, I don't recall exactly which things are blocked by the existence of postfix ops; but I think for instance it might become possible to remove the restriction of requiring AS before column aliases that happen to be unreserved keywords. If we lobotomize CREATE OPERATOR but don't remove built-in postfix ops, then none of those improvements will be available. That seems like the worst possible choice. I would also argue that having a feature that is available to built-in operators but not user-defined ones is pretty antithetical to Postgres philosophy. regards, tom lane
Re: factorial function/phase out postfix operators?
> > I won't lose a lot of sleep if we decide to rip out '!' as well, but I > don't think that continuing to support it would cost us much. > +1 for keeping ! and nuking the rest, if possible. Regards, Ken
Re: factorial function/phase out postfix operators?
út 19. 5. 2020 v 14:27 odesílatel Robert Haas napsal: > On Mon, May 18, 2020 at 10:42 AM Peter Eisentraut > wrote: > > What are the thoughts about then marking the postfix operator deprecated > > and eventually removing it? > > I wrote a little bit about this last year: > > > http://postgr.es/m/CA+TgmoarLfSQcLCh7jx0737SZ28qwbuy+rUWT6rSHAO=b-6...@mail.gmail.com > > I think it's generally a good idea, though perhaps we should consider > continuing to allow '!' as a postfix operator and just removing > support for any other. That would probably allow us to have a very > short deprecation period, since real-world use of user-defined postfix > operators seems to be nil -- and it would also make this into a change > that only affects the lexer and parser, which might make it simpler. > > I won't lose a lot of sleep if we decide to rip out '!' as well, but I > don't think that continuing to support it would cost us much. > This is little bit obscure feature. It can be removed and relative quickly. Maybe some warning if somebody use it can be good (for Postgres 13) Pavel > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > > >
Re: factorial function/phase out postfix operators?
On Mon, May 18, 2020 at 10:42 AM Peter Eisentraut wrote: > What are the thoughts about then marking the postfix operator deprecated > and eventually removing it? I wrote a little bit about this last year: http://postgr.es/m/CA+TgmoarLfSQcLCh7jx0737SZ28qwbuy+rUWT6rSHAO=b-6...@mail.gmail.com I think it's generally a good idea, though perhaps we should consider continuing to allow '!' as a postfix operator and just removing support for any other. That would probably allow us to have a very short deprecation period, since real-world use of user-defined postfix operators seems to be nil -- and it would also make this into a change that only affects the lexer and parser, which might make it simpler. I won't lose a lot of sleep if we decide to rip out '!' as well, but I don't think that continuing to support it would cost us much. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: factorial function/phase out postfix operators?
On 5/19/20 4:03 AM, Tom Lane wrote: > Peter Eisentraut writes: >> What are the thoughts about then marking the postfix operator deprecated >> and eventually removing it? > > If we do this it'd require a plan. We'd have to also warn about the > feature deprecation in (at least) the CREATE OPERATOR man page, and > we'd have to decide how many release cycles the deprecation notices > need to stand for. I have never come across any custom postfix operators in the wild, and I've never even seen ! used in practice. So I would suggest a very short deprecation period. Deprecate now in 13, let 14 go by, and rip it all out for 15. That should give us enough time to extend the deprecation period if we need to, or go back on it entirely (like I seem to remember we did with VACUUM FREEZE). > If that's the intention, though, it'd be good to get those deprecation > notices published in v13 not v14. +1 -- Vik Fearing
Re: factorial function/phase out postfix operators?
On Mon, May 18, 2020 at 10:03:13PM -0400, Tom Lane wrote: > Peter Eisentraut writes: > > What are the thoughts about then marking the postfix operator deprecated > > and eventually removing it? > > If we do this it'd require a plan. We'd have to also warn about the > feature deprecation in (at least) the CREATE OPERATOR man page, and > we'd have to decide how many release cycles the deprecation notices > need to stand for. > > If that's the intention, though, it'd be good to get those deprecation > notices published in v13 not v14. +1 for deprecating in v13. Best, David. -- David Fetter http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
Re: factorial function/phase out postfix operators?
Peter Eisentraut writes: > What are the thoughts about then marking the postfix operator deprecated > and eventually removing it? If we do this it'd require a plan. We'd have to also warn about the feature deprecation in (at least) the CREATE OPERATOR man page, and we'd have to decide how many release cycles the deprecation notices need to stand for. If that's the intention, though, it'd be good to get those deprecation notices published in v13 not v14. regards, tom lane
Re: factorial function/phase out postfix operators?
On Mon, May 18, 2020 at 05:02:34PM +0200, Vik Fearing wrote: > On 5/18/20 4:42 PM, Peter Eisentraut wrote: > > There have been occasional discussions about deprecating or phasing out > > postfix operators, to make various things easier in the parser. > > > > The first step would in any case be to provide alternatives for the > > existing postfix operators. There is currently one, namely the numeric > > factorial operator "!". A sensible alternative for that would be > > providing a function factorial(numeric) -- and that already exists but > > is not documented. (Note that the operator is mapped to proname > > "numeric_fac". The function "factorial" maps to the same prosrc but is > > otherwise independent of the operator.) > > > > So I suggest that we add that function to the documentation. > > I think this should be done regardless. > > > (Some adjacent cleanup work might also be in order. The test cases for > > factorial are currently in int4.sql, but all the factorial functionality > > was moved to numeric a long time ago.) > > > > What are the thoughts about then marking the postfix operator deprecated > > and eventually removing it? > > I am greatly in favor of removing postfix operators as soon as possible. Agreed. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com + As you are, so once was I. As I am, so you will be. + + Ancient Roman grave inscription +
Re: factorial function/phase out postfix operators?
On 5/18/20 4:42 PM, Peter Eisentraut wrote: > There have been occasional discussions about deprecating or phasing out > postfix operators, to make various things easier in the parser. > > The first step would in any case be to provide alternatives for the > existing postfix operators. There is currently one, namely the numeric > factorial operator "!". A sensible alternative for that would be > providing a function factorial(numeric) -- and that already exists but > is not documented. (Note that the operator is mapped to proname > "numeric_fac". The function "factorial" maps to the same prosrc but is > otherwise independent of the operator.) > > So I suggest that we add that function to the documentation. I think this should be done regardless. > (Some adjacent cleanup work might also be in order. The test cases for > factorial are currently in int4.sql, but all the factorial functionality > was moved to numeric a long time ago.) > > What are the thoughts about then marking the postfix operator deprecated > and eventually removing it? I am greatly in favor of removing postfix operators as soon as possible. -- Vik Fearing
factorial function/phase out postfix operators?
There have been occasional discussions about deprecating or phasing out postfix operators, to make various things easier in the parser. The first step would in any case be to provide alternatives for the existing postfix operators. There is currently one, namely the numeric factorial operator "!". A sensible alternative for that would be providing a function factorial(numeric) -- and that already exists but is not documented. (Note that the operator is mapped to proname "numeric_fac". The function "factorial" maps to the same prosrc but is otherwise independent of the operator.) So I suggest that we add that function to the documentation. (Some adjacent cleanup work might also be in order. The test cases for factorial are currently in int4.sql, but all the factorial functionality was moved to numeric a long time ago.) What are the thoughts about then marking the postfix operator deprecated and eventually removing it? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
