Thread: 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
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
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 <bruce@momjian.us> 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 +
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
On Mon, May 18, 2020 at 10:03:13PM -0400, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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 <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
On 5/19/20 4:03 AM, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
On Mon, May 18, 2020 at 10:42 AM Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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-6xdw@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
út 19. 5. 2020 v 14:27 odesílatel Robert Haas <robertmhaas@gmail.com> napsal:
On Mon, May 18, 2020 at 10:42 AM Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> 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-6xdw@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
> > 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
Robert Haas <robertmhaas@gmail.com> 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
On Tue, May 19, 2020 at 9:51 AM Tom Lane <tgl@sss.pgh.pa.us> 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
On 5/19/20 4:22 PM, Robert Haas wrote: > On Tue, May 19, 2020 at 9:51 AM Tom Lane <tgl@sss.pgh.pa.us> 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
On Tue, May 19, 2020 at 10:36 AM Vik Fearing <vik@postgresfriends.org> wrote: > 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. Fair enough. I think you may be in the majority on that one, too. I just wanted to raise the issue, and we'll see if anyone else agrees. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Robert Haas <robertmhaas@gmail.com> 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
Greetings, * Vik Fearing (vik@postgresfriends.org) wrote: > On 5/19/20 4:03 AM, Tom Lane wrote: > > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
Attachment
Vik Fearing <vik@postgresfriends.org> 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
On Tue, May 19, 2020 at 11:32 AM Tom Lane <tgl@sss.pgh.pa.us> 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-LyG3w2wOQ@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
Robert Haas <robertmhaas@gmail.com> writes: > On Tue, May 19, 2020 at 11:32 AM Tom Lane <tgl@sss.pgh.pa.us> 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-LyG3w2wOQ@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
On Tue, May 19, 2020 at 2:30 PM Tom Lane <tgl@sss.pgh.pa.us> 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
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, May 19, 2020 at 2:30 PM Tom Lane <tgl@sss.pgh.pa.us> 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
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 <str> Sconst comment_text notify_payload
%type <str> RoleId opt_boolean_or_string
%type <list> var_list
-%type <str> ColId ColLabel var_name type_function_name param_name
+%type <str> ColId ColLabel BareColLabel
%type <str> NonReservedWord NonReservedWord_or_Sconst
+%type <str> var_name type_function_name param_name
%type <str> createdb_opt_name
%type <node> var_value zone_value
%type <rolespec> auth_ident RoleSpec opt_granted_by
-%type <keyword> unreserved_keyword type_func_name_keyword
+%type <keyword> unreserved_keyword non_label_keyword type_func_name_keyword
%type <keyword> col_name_keyword reserved_keyword
%type <node> 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 { $$ = pstrdup($1); }
+ ;
+
/* Column label --- allowed labels in "AS" clauses.
* This presently includes *all* Postgres keywords.
*/
ColLabel: IDENT { $$ = $1; }
| unreserved_keyword { $$ = pstrdup($1); }
+ | non_label_keyword { $$ = pstrdup($1); }
| col_name_keyword { $$ = pstrdup($1); }
| type_func_name_keyword { $$ = pstrdup($1); }
| reserved_keyword { $$ = pstrdup($1); }
@@ -15326,7 +15333,6 @@ unreserved_keyword:
| CYCLE
| DATA_P
| DATABASE
- | DAY_P
| DEALLOCATE
| DECLARE
| DEFAULTS
@@ -15360,7 +15366,6 @@ unreserved_keyword:
| EXTENSION
| EXTERNAL
| FAMILY
- | FILTER
| FIRST_P
| FOLLOWING
| FORCE
@@ -15374,7 +15379,6 @@ unreserved_keyword:
| HANDLER
| HEADER_P
| HOLD
- | HOUR_P
| IDENTITY_P
| IF_P
| IMMEDIATE
@@ -15414,10 +15418,8 @@ unreserved_keyword:
| MATERIALIZED
| MAXVALUE
| METHOD
- | MINUTE_P
| MINVALUE
| MODE
- | MONTH_P
| MOVE
| NAME_P
| NAMES
@@ -15443,7 +15445,6 @@ unreserved_keyword:
| OPTIONS
| ORDINALITY
| OTHERS
- | OVER
| OVERRIDING
| OWNED
| OWNER
@@ -15499,7 +15500,6 @@ unreserved_keyword:
| SCHEMAS
| SCROLL
| SEARCH
- | SECOND_P
| SECURITY
| SEQUENCE
| SEQUENCES
@@ -15557,23 +15557,36 @@ unreserved_keyword:
| VALIDATE
| VALIDATOR
| VALUE_P
- | VARYING
| VERSION_P
| VIEW
| VIEWS
| VOLATILE
| WHITESPACE_P
- | WITHIN
- | WITHOUT
| WORK
| WRAPPER
| WRITE
| XML_P
- | YEAR_P
| YES_P
| ZONE
;
+/* "Non label" keywords --- cannot be a bare column label in a SELECT list
+ * (you have to write AS in front). Otherwise usable for anything.
+ */
+non_label_keyword:
+ DAY_P
+ | FILTER
+ | HOUR_P
+ | MINUTE_P
+ | MONTH_P
+ | OVER
+ | SECOND_P
+ | VARYING
+ | WITHIN
+ | WITHOUT
+ | YEAR_P
+ ;
+
/* Column identifier --- keywords that can be column, table, etc names.
*
* Many of these keywords will in fact be recognized as type or function
On Tue, May 19, 2020 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> 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
Robert Haas <robertmhaas@gmail.com> writes:
> On Tue, May 19, 2020 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> 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
> On May 20, 2020, at 11:24 AM, Tom Lane <tgl@sss.pgh.pa.us> 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
charactersthat are allowed in postfix operators vs. those allowed for infix operators, then we'd get to have postfix
operatorswithout 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
todisallow 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
operatorsnamed 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
Mark Dilger <mark.dilger@enterprisedb.com> writes:
> ... But if we made a clean distinction between the characters that are allowed in postfix operators vs. those allowed
forinfix 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
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 Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Alvaro Herrera <alvherre@2ndquadrant.com> 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
On Wed, May 20, 2020 at 2:24 PM Tom Lane <tgl@sss.pgh.pa.us> 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.
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
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
> On May 19, 2020, at 4:47 PM, Tom Lane <tgl@sss.pgh.pa.us> 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 matcheswhat 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. I didn't see much point in that. The way Tom had it in his patch was easy to work with. Maybe I'm missing something? The patch, attached, still needs documentation updates and an update to pg_upgrade. Users upgrading to v14 may have custompostfix operators. Should pg_upgrade leave them untouched? They wouldn't be reachable through the grammar any longer. Should pg_upgrade delete them? I'm generally not in favor of deleting user data as part of an upgrade, and rowsin the catalog tables corresponding to custom postfix operators are sort of user data, if you squint and look at themjust right. Thoughts? — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
> On Jun 30, 2020, at 2:47 PM, Mark Dilger <mark.dilger@enterprisedb.com> wrote: > > > >> On May 19, 2020, at 4:47 PM, Tom Lane <tgl@sss.pgh.pa.us> 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 matcheswhat 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 theAS keyword up to 411, with only 39 keywords still requiring an explicit preceding AS. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Sat, Jul 11, 2020 at 1:14 AM Mark Dilger <mark.dilger@enterprisedb.com> 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 withoutthe 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 Naylor https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On Jul 18, 2020, at 1:00 AM, John Naylor <john.naylor@2ndquadrant.com> wrote: > > On Sat, Jul 11, 2020 at 1:14 AM Mark Dilger > <mark.dilger@enterprisedb.com> 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 withoutthe 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: Thanks for looking! > By making col-label-ness an orthogonal attribute, do we still need the > category of non_label_keyword? It seems not. You are right. The non_label_keyword category has been removed from v3. > 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? 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 makesit harder for a person adding a new keyword to know what to place there. If they guess "FALSE", and also don't knowabout adding the new keyword to the bare_label_keyword rule in gram.y, then those two mistakes will agree with each otherand the person adding the keyword won't likely know they did it wrong. It is simple enough for gen_keywordlist.pl toconvert between what we use in kwlist.h and a boolean value for kwlist_d.h, so I did it that way. > 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 gowith 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. > Likewise, it seems the actual removal of postfix operators should be a > separate patch. I broke out the removal of postfix operators into its own patch in the v3 series. This patch does not attempt to remove pre-existing postfix operators from existing databases, so users upgrading to the newmajor 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. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Wed, Jul 22, 2020 at 8:47 AM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
>
>
>
> > On Jul 18, 2020, at 1:00 AM, John Naylor <john.naylor@2ndquadrant.com> 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
makesit harder for a person adding a new keyword to know what to place there. If they guess "FALSE", and also don't
knowabout adding the new keyword to the bare_label_keyword rule in gram.y, then those two mistakes will agree with each
otherand the person adding the keyword won't likely know they did it wrong. It is simple enough for gen_keywordlist.pl
toconvert 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'tgo 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
thenew 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 Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Tue, May 19, 2020 at 5:03 AM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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 Naylor https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On Aug 24, 2020, at 12:28 AM, John Naylor <john.naylor@2ndquadrant.com> wrote:
>
> On Tue, May 19, 2020 at 5:03 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>
>> Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
> On Jul 29, 2020, at 5:03 AM, John Naylor <john.naylor@2ndquadrant.com> wrote:
>
> On Wed, Jul 22, 2020 at 8:47 AM Mark Dilger
> <mark.dilger@enterprisedb.com> wrote:
>>
>>
>>
>>> On Jul 18, 2020, at 1:00 AM, John Naylor <john.naylor@2ndquadrant.com> 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.
Likewise, John. Thanks for the review! I am attaching version 4 of the patch to address your comments.
>> 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
makesit harder for a person adding a new keyword to know what to place there. If they guess "FALSE", and also don't
knowabout adding the new keyword to the bare_label_keyword rule in gram.y, then those two mistakes will agree with each
otherand the person adding the keyword won't likely know they did it wrong. It is simple enough for gen_keywordlist.pl
toconvert 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'tgo 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".
The construction colname AS colalias brings to mind the words "pseudonym" and "alias". The distinction we're trying to
drawhere 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
"implicitalias" 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.
I've replaced uses of "bare" with "implicit" or "implicit_alias" or similar.
>
>> This patch does not attempt to remove pre-existing postfix operators from existing databases, so users upgrading to
thenew 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.
Done. Testing an upgrade of a 9.1 test install, relying on the regression database having left over user defined
postfixoperators, gives this result:
pg_upgrade --old-bindir=/Users/mark.dilger/pg91/bin --old-datadir=/Users/mark.dilger/pg91/test_data
--new-bindir=/Users/mark.dilger/pgtest/test_install/bin--new-datadir=/Users/mark.dilger/pgtest/test_data
Performing Consistency Checks
-----------------------------
Checking cluster versions ok
Checking database user is the install user ok
Checking database connection settings ok
Checking for prepared transactions ok
Checking for reg* data types in user tables ok
Checking for contrib/isn with bigint-passing mismatch ok
Checking for user defined postfix operators fatal
Your installation contains user defined postfix operators, which is not
supported anymore. Consider dropping the postfix operators and replacing
them with prefix operators or function calls.
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
thereis a preferred way to lay out that information.
>
> 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.
Changed the hint to say "Postfix operators are not supported."
Changed the regression test comment and code comments to not use the objectionable language you mention.
> +SELECT -5!;
> +SELECT -0!;
> +SELECT 0!;
> +SELECT 100!;
>
> I think one negative and one non-negative case is enough to confirm
> the syntax error.
Ok, done.
> - gram.y still contains "POSTFIXOP" and "postfix-operator".
>
> - parse_expr.c looks like it has some now-unreachable code.
Good point. I've removed or renamed that stuff. Some of it went away, but some stuff was shared between general
postfixoperators and ANY/ALL, so that just got renamed accordingly.
> 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 or present is that we're making
analias, so "implicit alias" and "explicit alias" sound correct to me.
> - * kwlist.h's table from one source of truth.)
> + * kwlist.h's table from a common master list.)
>
> Off topic.
Removed. This appears to have been an unintentional revert of an unrelated commit.
> 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.
Fixed.
> Continuing on, I think 2 and 3 can be squashed together. If anything,
> it should make revisiting cosmetic decisions easier.
Squashed together.
> + 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.
I've changed it from "bool bare" to "text aliastype". (I still wish "plicity" were a word.) Rather than true/false,
itreturns "implicit"/"explicit".
> - 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.
Well, I did touch that function a bit, adding a new column, and the number of rows returned is exactly 450, so if I'm
notgoing to update it, who will? The count may increase over time if other keywords are added, but I doubt anybody who
addsa single keyword would bother updating prorows here.
I agree that it doesn't matter much. If you don't buy into the paragraph above, I'll remove it for the next patch
version.
> "EXPLICIT_LABEL" -- continuing my line of thought above, all labels
> are explicit, that's why they're called labels.
Right. Labels are explicit.
> Brainstorm:
>
> EXPLICIT_AS_LABEL
> EXPLICIT_AS
> NON_BARE_LABEL
> *shrug*
Changed to EXPLICIT_ALIAS.
> + # 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.
Done.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachment
On Wed, Aug 26, 2020 at 6:12 AM Mark Dilger
<mark.dilger@enterprisedb.com> wrote:
>
> The construction colname AS colalias brings to mind the words "pseudonym" and "alias". The distinction we're trying
todraw 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
"implicitalias" 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
thereis 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 or present is that we're making
analias, so "implicit alias" and "explicit alias" sound correct to me.
Sounds fine.
> > - 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.
>
> Well, I did touch that function a bit, adding a new column, and the number of rows returned is exactly 450, so if I'm
notgoing to update it, who will? The count may increase over time if other keywords are added, but I doubt anybody who
addsa single keyword would bother updating prorows here.
>
> I agree that it doesn't matter much. If you don't buy into the paragraph above, I'll remove it for the next patch
version.
No strong feelings -- if it were me, I'd put in a separate
"by-the-way" patch at the end, and the committer can squash at their
discretion. But not really worth a separate thread.
# select aliastype, count(*) from pg_get_keywords() group by 1;
aliastype | count
-----------+-------
explicit | 39
implicit | 411
(2 rows)
Nice!
The binary has increased by ~16kB, mostly because of the new keyword
list in the grammar, but that's pretty small, all things considered.
--
John Naylor https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
> On Aug 26, 2020, at 6:33 AM, John Naylor <john.naylor@2ndquadrant.com> wrote:
>
> + /*
> + * 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.
I don't want to reorder the code, but combining the two code comments together allows the comment to flow more as you
indicate. Done for v5.
>
> - * 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",
Right. I have fixed the comment. Thanks for noticing.
> 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
followingcleanup 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.
>
> -{ 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.
Yeah, I don't have strong feelings about this. We should decide soon, though, because we should have the deprecation
warningsresolved in time for v13, even if this patch hasn't been applied yet.
> 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... */
Cleaned up.
>>> 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 or present is that we're
makingan alias, so "implicit alias" and "explicit alias" sound correct to me.
>
> Sounds fine.
>
>>> - 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.
>>
>> Well, I did touch that function a bit, adding a new column, and the number of rows returned is exactly 450, so if
I'mnot going to update it, who will? The count may increase over time if other keywords are added, but I doubt anybody
whoadds a single keyword would bother updating prorows here.
>>
>> I agree that it doesn't matter much. If you don't buy into the paragraph above, I'll remove it for the next patch
version.
>
> No strong feelings -- if it were me, I'd put in a separate
> "by-the-way" patch at the end, and the committer can squash at their
> discretion. But not really worth a separate thread.
Ok, I've split this out into
>
> # select aliastype, count(*) from pg_get_keywords() group by 1;
> aliastype | count
> -----------+-------
> explicit | 39
> implicit | 411
> (2 rows)
>
> Nice!
I think the number of people porting from other RDBMSs to PostgreSQL who are helped by this patch scales with the
numberof keywords moved to the "implicit" category, and we've done pretty well here.
I wonder if we can get more comments for or against this patch, at least in principle, in the very near future, to help
determinewhether the deprecation notices should go into v13?
> The binary has increased by ~16kB, mostly because of the new keyword
> list in the grammar, but that's pretty small, all things considered.
Right, thanks for checking the size increase.
—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Attachment
On Wed, Aug 26, 2020 at 11:57 AM Mark Dilger <mark.dilger@enterprisedb.com> wrote: > I wonder if we can get more comments for or against this patch, at least in principle, in the very near future, to helpdetermine 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
On Wed, Aug 26, 2020 at 6:57 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote: > > > > > On Aug 26, 2020, at 6:33 AM, John Naylor <john.naylor@2ndquadrant.com> 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 followingcleanup 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 Naylor https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Aug 26, 2020 at 8:55 PM Robert Haas <robertmhaas@gmail.com> wrote: > > On Wed, Aug 26, 2020 at 11:57 AM Mark Dilger > <mark.dilger@enterprisedb.com> wrote: > > I wonder if we can get more comments for or against this patch, at least in principle, in the very near future, to helpdetermine 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 <note> tag to make it more visible, like in the attached, compressed to avoid confusing the cfbot. -- John Naylor https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachment
On Thu, Aug 27, 2020 at 7:12 AM John Naylor <john.naylor@2ndquadrant.com> wrote: > Well, for starters it'll say the obvious, but since we have a concrete > timeframe, maybe a <note> 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
Robert Haas <robertmhaas@gmail.com> 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
> On Aug 27, 2020, at 7:04 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> 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 werechoosing one operator per underlying proc to be the canonical operator name, and deprecating all other operators basedon the same proc. You chose postfix ! as the canonical operator for numeric_fac and deprecated prefix !!, but I thinkI can infer from that commit that if postfix ! did not exist, prefix !! would have been the canonical operator and wouldnot 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
On Thu, Aug 27, 2020 at 10:04 AM Tom Lane <tgl@sss.pgh.pa.us> 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
On Thu, Aug 27, 2020 at 5:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > I'm a bit inclined to kill them both off and standardize on factorial() > (not numeric_fac). +1 -- John Naylor https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Aug 26, 2020 at 6:57 PM Mark Dilger <mark.dilger@enterprisedb.com> 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 followingcleanup 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 Naylor https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Fri, Aug 28, 2020 at 4:44 AM John Naylor <john.naylor@2ndquadrant.com> wrote: > On Thu, Aug 27, 2020 at 5:04 PM Tom Lane <tgl@sss.pgh.pa.us> 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
Attachment
On Fri, Aug 28, 2020 at 11:00 AM Robert Haas <robertmhaas@gmail.com> wrote: > On Fri, Aug 28, 2020 at 4:44 AM John Naylor <john.naylor@2ndquadrant.com> wrote: > > On Thu, Aug 27, 2020 at 5:04 PM Tom Lane <tgl@sss.pgh.pa.us> 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
> On Aug 27, 2020, at 2:24 AM, John Naylor <john.naylor@2ndquadrant.com> wrote: > > On Wed, Aug 26, 2020 at 6:57 PM Mark Dilger > <mark.dilger@enterprisedb.com> wrote: >> >> >> >>> On Aug 26, 2020, at 6:33 AM, John Naylor <john.naylor@2ndquadrant.com> 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 followingcleanup 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. You wrote in a later email: > 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. So I tried to leave the precedence stuff alone as much as possible in this next patch set. I agree such refactoring canbe done separately, and at a later time. > > 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." Some changes were made on another thread [1] for the deprecation notices, committed recently by Tom, and I think this patchset is compatible with what was done there. This patch set is intended for commit against master, targeted for PostgreSQL14, so the deprecation notices are removed along with the things that were deprecated. The references to right-unaryoperators that you call out, above, have been removed. [1] https://postgr.es/m/BE2DF53D-251A-4E26-972F-930E523580E9@enterprisedb.com — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Tue, Sep 1, 2020 at 10:00 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote: > > Some changes were made on another thread [1] for the deprecation notices, committed recently by Tom, and I think this patchset is compatible with what was done there. This patch set is intended for commit against master, targeted for PostgreSQL14, so the deprecation notices are removed along with the things that were deprecated. The references to right-unaryoperators 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 Naylor https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
> On Sep 2, 2020, at 1:33 AM, John Naylor <john.naylor@2ndquadrant.com> wrote: > > On Tue, Sep 1, 2020 at 10:00 PM Mark Dilger > <mark.dilger@enterprisedb.com> wrote: >> >> Some changes were made on another thread [1] for the deprecation notices, committed recently by Tom, and I think thispatch set is compatible with what was done there. This patch set is intended for commit against master, targeted forPostgreSQL 14, so the deprecation notices are removed along with the things that were deprecated. The references to right-unaryoperators 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. Right you are. Removed in v7. > 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. Since newer pg_dump binaries can be used to dump data from older servers, and since users might then load that dump backinto an older server, I think doing anything stronger than a pg_log_warning() would be incorrect. I did not find precedentsunder 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. A pg_log_warning has been added in v7. Dumping right-unary (postfix) operators should work (with a warning) in v7. I think pg_dump in v6 was broken in this regard. > Nitpick: these can be removed, since we already test factorial() in this file: > > -SELECT 4!; > -SELECT !!3; > +SELECT factorial(4); > +SELECT factorial(3); I was on the fence between removing those (as you suggest) vs. converting them to function calls (as v6 did). They are removedin v7. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Attachment
On Thu, Sep 3, 2020 at 11:50 AM Mark Dilger <mark.dilger@enterprisedb.com> wrote: > [v7] Ok, I've marked it ready for committer. -- John Naylor https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Thu, Sep 3, 2020 at 11:50 AM Mark Dilger <mark.dilger@enterprisedb.com> wrote: > Since newer pg_dump binaries can be used to dump data from older servers, and since users might then load that dump backinto an older server, I think doing anything stronger than a pg_log_warning() would be incorrect. I did not find precedentsunder 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
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Sep 3, 2020 at 11:50 AM Mark Dilger
> <mark.dilger@enterprisedb.com> wrote:
>> Since newer pg_dump binaries can be used to dump data from older servers, and since users might then load that dump
backinto an older server, I think doing anything stronger than a pg_log_warning() would be incorrect. I did not find
precedentsunder 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
> On Sep 11, 2020, at 8:36 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Robert Haas <robertmhaas@gmail.com> writes: >> On Thu, Sep 3, 2020 at 11:50 AM Mark Dilger >> <mark.dilger@enterprisedb.com> wrote: >>> Since newer pg_dump binaries can be used to dump data from older servers, and since users might then load that dump backinto an older server, I think doing anything stronger than a pg_log_warning() would be incorrect. I did not find precedentsunder 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 acrossan expensive link, or if they intend to blow away the old data directory to make room for the new. Whether that indicationshould be in the form of a warning or an error is less clear to me. Whatever we do here, I think it sets a precedentfor how such situations are handled in the future, so maybe focusing overmuch on the postfix operator issue is lesshelpful than on the broader concept. What, for example, would we do if we someday dropped GiST support? Print a warningwhen 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 loadinto PostgreSQL server versions newer than pg_dump's version. <snip> > Also, it is not guaranteed that pg_dump's output can be loaded into a server of an older major version — not even if thedump 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 thathas been dropped from the newer major version. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Mark Dilger <mark.dilger@enterprisedb.com> writes:
> On Sep 11, 2020, at 8:36 AM, Tom Lane <tgl@sss.pgh.pa.us> 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
> On Sep 11, 2020, at 11:25 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Mark Dilger <mark.dilger@enterprisedb.com> writes: >> On Sep 11, 2020, at 8:36 AM, Tom Lane <tgl@sss.pgh.pa.us> 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 todo with postfix operators specifically, but be more general like --dump-incompatible-objects vs. --omit-incompatible-objectsvs. --error-on-incompatible-objects vs. --do-your-best-to-fixup-incompatible-objects, with oneof 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 forcomment. — Mark Dilger EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Sep 11, 2020 at 3:23 PM Mark Dilger <mark.dilger@enterprisedb.com> wrote: > Another option would be to have pg_dump take a strictness mode option. I don't think the option should have anything todo with postfix operators specifically, but be more general like --dump-incompatible-objects vs. --omit-incompatible-objectsvs. --error-on-incompatible-objects vs. --do-your-best-to-fixup-incompatible-objects, with oneof 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 listfor 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
> On Sep 11, 2020, at 12:54 PM, Robert Haas <robertmhaas@gmail.com> wrote: > > On Fri, Sep 11, 2020 at 3:23 PM Mark Dilger > <mark.dilger@enterprisedb.com> wrote: >> Another option would be to have pg_dump take a strictness mode option. I don't think the option should have anythingto do with postfix operators specifically, but be more general like --dump-incompatible-objects vs. --omit-incompatible-objectsvs. --error-on-incompatible-objects vs. --do-your-best-to-fixup-incompatible-objects, with oneof 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 listfor 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
Mark Dilger <mark.dilger@enterprisedb.com> writes:
> On Sep 11, 2020, at 12:54 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> On Fri, Sep 11, 2020 at 3:23 PM Mark Dilger
>> <mark.dilger@enterprisedb.com> wrote:
>>> Another option would be to have pg_dump take a strictness mode option. I don't think the option should have
anythingto do with postfix operators specifically, but be more general like --dump-incompatible-objects vs.
--omit-incompatible-objectsvs. --error-on-incompatible-objects vs. --do-your-best-to-fixup-incompatible-objects, with
oneof 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
listfor 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
Robert Haas <robertmhaas@gmail.com> 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
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
> On Sep 18, 2020, at 8:29 AM, Tom Lane <tgl@sss.pgh.pa.us> 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
On Fri, Sep 18, 2020 at 11:29 AM Tom Lane <tgl@sss.pgh.pa.us> 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
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Sep 18, 2020 at 11:29 AM Tom Lane <tgl@sss.pgh.pa.us> 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
On Fri, Sep 18, 2020 at 2:11 PM Tom Lane <tgl@sss.pgh.pa.us> 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
Robert Haas <robertmhaas@gmail.com> writes:
> On Fri, Sep 18, 2020 at 2:11 PM Tom Lane <tgl@sss.pgh.pa.us> 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
Pushed with the discussed terminological changes and some other
fooling about, including fixing the documentation.
regards, tom lane
On Fri, Sep 18, 2020 at 4:48 PM Tom Lane <tgl@sss.pgh.pa.us> 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
John Naylor <john.naylor@2ndquadrant.com> 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