Horton

Thread

Commits

  1. Remove precedence hacks no longer needed without postfix operators.

  2. Allow most keywords to be used as column labels without requiring AS.

  3. Remove support for postfix (right-unary) operators.

  4. Remove factorial operators, leaving only the factorial() function.

  5. Further refine patch for commenting operator implementation functions.

  1. factorial function/phase out postfix operators?

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-05-18T14:42:18Z

    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

  2. Re: factorial function/phase out postfix operators?

    Vik Fearing <vik@postgresfriends.org> — 2020-05-18T15:02:34Z

    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

  3. Re: factorial function/phase out postfix operators?

    Bruce Momjian <bruce@momjian.us> — 2020-05-18T15:39:34Z

    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 +

  4. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-19T02:03:13Z

    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

  5. Re: factorial function/phase out postfix operators?

    David Fetter <david@fetter.org> — 2020-05-19T02:11:37Z

    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

  6. Re: factorial function/phase out postfix operators?

    Vik Fearing <vik@postgresfriends.org> — 2020-05-19T02:30:23Z

    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

  7. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-05-19T12:27:39Z

    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

  8. Re: factorial function/phase out postfix operators?

    Pavel Stehule <pavel.stehule@gmail.com> — 2020-05-19T12:33:38Z

    ú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
>
>
>

  9. Re: factorial function/phase out postfix operators?

    ktm@rice.edu <ktm@rice.edu> — 2020-05-19T13:28:34Z

    > 
> 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

  10. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-19T13:51:36Z

    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

  11. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-05-19T14:22:35Z

    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

  12. Re: factorial function/phase out postfix operators?

    Vik Fearing <vik@postgresfriends.org> — 2020-05-19T14:36:13Z

    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

  13. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-05-19T14:39:48Z

    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

  14. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-19T14:48:22Z

    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

  15. Re: factorial function/phase out postfix operators?

    Stephen Frost <sfrost@snowman.net> — 2020-05-19T14:54:49Z

    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

  16. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-19T15:32:10Z

    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

  17. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-05-19T18:03:21Z

    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

  18. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-19T18:30:08Z

    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

  19. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-05-19T19:31:03Z

    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

  20. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-19T19:50:41Z

    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

  21. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-19T23:47:11Z

    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

  22. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-05-20T16:32:34Z

    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

  23. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-20T18:24:43Z

    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

  24. Re: factorial function/phase out postfix operators?

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-05-20T20:18:01Z

    
> 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 characters that are allowed in postfix operators vs. those allowed for infix operators, then we'd get to have postfix operators without the ambiguity, right?

When thinking about postfix operators, the subscript and superscript character ranges come to my mind, such as

	SELECT Σ₂(x² + y³ + z⁴);

These also come to mind as prefix operators, but I don't recall seeing them as infix operators, so maybe it would be ok to disallow that?  As for the ! infix operator, it doesn't exist by default:

+SELECT x ! y from (select 5 AS x, 3 AS y) AS ss;
+ERROR:  operator does not exist: integer ! integer
+LINE 1: SELECT x ! y from (select 5 AS x, 3 AS y) AS ss;
+                 ^
+HINT:  No operator matches the given name and argument types. You might need to add explicit type casts.

So if we put that in the set of characters disallowed for infix operators, we would only be breaking custom infix operators named that, which seems like less breakage to me than removing postfix operators of all kinds.


—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  25. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-20T20:29:15Z

    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 for infix operators, then we'd get to have postfix operators without the ambiguity, right?

I continue to see little point in half-baked compatibility measures
like that.  You'd be much more likely to break working setups (that
might not even involve any postfix operators) than to accomplish
anything useful.  In particular, if Joe DBA out there has a postfix
operator, and it's not named according to whatever rule you chose,
then you haven't done anything to fix his compatibility problem.

> When thinking about postfix operators, the subscript and superscript character ranges come to my mind, such as
> 	SELECT Σ₂(x² + y³ + z⁴);

We already have a convention about non-ASCII characters, and it is that
they are identifier characters not operator characters.  Changing that
would break yet a different set of applications.  (That is to say,
the above SELECT already has a well-defined lexical interpretation.)

			regards, tom lane

  26. Re: factorial function/phase out postfix operators?

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-05-20T21:54:18Z

    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

  27. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-20T22:21:17Z

    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

  28. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-05-21T13:36:43Z

    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.

  29. Re: factorial function/phase out postfix operators?

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-05-25T20:50:48Z

    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

  30. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-05-25T22:18:25Z

    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

  31. Re: factorial function/phase out postfix operators?

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-06-30T21:47:24Z

    
> 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 matches what Robert said upthread.

Tom and Álvaro discussed upthread:

> Would it make sense (and possible) to have a keyword category that is
> not disjoint wrt. the others?  Maybe that ends up being easier than
> a solution that ends up with six or seven categories.

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 custom postfix 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 rows in the catalog tables corresponding to custom postfix operators are sort of user data, if you squint and look at them just right.  Thoughts?

  32. Re: factorial function/phase out postfix operators?

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-07-10T17:13:55Z

    
> 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 matches what Robert said upthread.
> 
> Tom and Álvaro discussed upthread:
> 
>> Would it make sense (and possible) to have a keyword category that is
>> not disjoint wrt. the others?  Maybe that ends up being easier than
>> a solution that ends up with six or seven categories.

Version 2, attached, follows this design, increasing the number of keywords that can be used as column aliases without the AS keyword up to 411, with only 39 keywords still requiring an explicit preceding AS.

  33. Re: factorial function/phase out postfix operators?

    John Naylor <john.naylor@2ndquadrant.com> — 2020-07-18T08:00:12Z

    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 without the AS keyword up to 411, with only 39 keywords still requiring an explicit preceding AS.

Hi Mark,

This isn't a full review, but I have a few questions/comments:

By making col-label-ness an orthogonal attribute, do we still need the
category of non_label_keyword? It seems not.

pg_get_keywords() should probably have a column to display ability to
act as a bare col label. Perhaps a boolean? If so, what do you think
of using true/false for the new field in kwlist.h as well?

In the bikeshedding department, it seems "implicit" was chosen because
it was distinct from "bare". I think "bare" as a descriptor should be
kept throughout for readability's sake. Maybe BareColLabel could be
"IDENT or bare_label_keyword" for example. Same for the $status var.

Likewise, it seems the actual removal of postfix operators should be a
separate patch.

-- 
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  34. Re: factorial function/phase out postfix operators?

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-07-22T00:46:59Z

    
> 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 without the AS keyword up to 411, with only 39 keywords still requiring an explicit preceding AS.
> 
> Hi Mark,
> 
> This isn't a full review, but I have a few questions/comments:

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 makes it harder for a person adding a new keyword to know what to place there.  If they guess "FALSE", and also don't know about adding the new keyword to the bare_label_keyword rule in gram.y, then those two mistakes will agree with each other and the person adding the keyword won't likely know they did it wrong.  It is simple enough for gen_keywordlist.pl to convert between what we use in kwlist.h and a boolean value for kwlist_d.h, so I did it that way.

> In the bikeshedding department, it seems "implicit" was chosen because
> it was distinct from "bare". I think "bare" as a descriptor should be
> kept throughout for readability's sake. Maybe BareColLabel could be
> "IDENT or bare_label_keyword" for example. Same for the $status var.

The category "bare_label_keyword" is used in v3.  As for the $status var, I don't want to name that $bare, as I didn't go with your idea about using a boolean.  $status = "BARE_LABEL" vs "EXPLICIT_LABEL" makes sense to me, more than $bare = "BARE_LABEL" vs "EXPLICIT_LABEL" does.  "status" is still a bit vague, so more bikeshedding is welcome.

> 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 new major version who have custom postfix operators will find that pg_upgrade chokes trying to recreate the postfix operator.  That's not great, but perhaps there is nothing automated that we could do for them that would be any better.

  35. Re: factorial function/phase out postfix operators?

    John Naylor <john.naylor@2ndquadrant.com> — 2020-07-29T12:03:35Z

    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 makes it harder for a person adding a new keyword to know what to place there.  If they guess "FALSE", and also don't know about adding the new keyword to the bare_label_keyword rule in gram.y, then those two mistakes will agree with each other and the person adding the keyword won't likely know they did it wrong.  It is simple enough for gen_keywordlist.pl to convert between what we use in kwlist.h and a boolean value for kwlist_d.h, so I did it that way.

Sounds fine to me.

> > In the bikeshedding department, it seems "implicit" was chosen because
> > it was distinct from "bare". I think "bare" as a descriptor should be
> > kept throughout for readability's sake. Maybe BareColLabel could be
> > "IDENT or bare_label_keyword" for example. Same for the $status var.
>
> The category "bare_label_keyword" is used in v3.  As for the $status var, I don't want to name that $bare, as I didn't go with your idea about using a boolean.  $status = "BARE_LABEL" vs "EXPLICIT_LABEL" makes sense to me, more than $bare = "BARE_LABEL" vs "EXPLICIT_LABEL" does.  "status" is still a bit vague, so more bikeshedding is welcome.

Yeah, it's very generic, but it's hard to find a short word for
"can-be-used-as-a-bare-column-label-ness".

> This patch does not attempt to remove pre-existing postfix operators from existing databases, so users upgrading to the new major version who have custom postfix operators will find that pg_upgrade chokes trying to recreate the postfix operator.  That's not great, but perhaps there is nothing automated that we could do for them that would be any better.

I'm thinking it would be good to have something like

select oid from pg_operator where oprright = 0 and oid >= FirstNormalObjectId;

in the pre-upgrade check.

Other comments:

0001:

+ errhint("postfix operator support has been discontinued")));

This language seems more appropriate for release notes -- I would word
the hint in the present, as in "postfix operators are not supported".
Ditto the words "discontinuation", "has been removed", and "no longer
works" elsewhere in the patch.

+SELECT -5!;
+SELECT -0!;
+SELECT 0!;
+SELECT 100!;

I think one negative and one non-negative case is enough to confirm
the syntax error.

- gram.y still contains "POSTFIXOP" and "postfix-operator".

- parse_expr.c looks like it has some now-unreachable code.


0002:

+ * All keywords can be used explicitly as a column label in expressions
+ * like 'SELECT 1234 AS keyword', but only some keywords can be used
+ * implicitly as column labels in expressions like 'SELECT 1234 keyword'.
+ * Those that can be used implicitly should be listed here.

In my mind, "AS" is the thing that's implied when not present, so we
should reword this to use the "bare" designation when talking about
the labels. I think there are contexts elsewhere where the implicit
column label is "col1, col2, col3...". I can't remember offhand where
that is though.

- * kwlist.h's table from one source of truth.)
+ * kwlist.h's table from a common master list.)

Off topic.


0003:

First off, I get a crash when trying

select * from pg_get_keywords();

and haven't investigated further. I don't think the returned types
match, though.

Continuing on, I think 2 and 3 can be squashed together. If anything,
it should make revisiting cosmetic decisions easier.

+ TupleDescInitEntry(tupdesc, (AttrNumber) 4, "bare",
+    BOOLOID, -1, 0);

Perhaps something a bit meatier for the user-visible field name. I
don't have a great suggestion.

-  proname => 'pg_get_keywords', procost => '10', prorows => '400',
+  proname => 'pg_get_keywords', procost => '10', prorows => '450',

Off topic for this patch. Not sure it matters much, either.

"EXPLICIT_LABEL" -- continuing my line of thought above, all labels
are explicit, that's why they're called labels. Brainstorm:

EXPLICIT_AS_LABEL
EXPLICIT_AS
NON_BARE_LABEL
*shrug*

+ # parser/kwlist.h lists each keyword as either bare or
+ # explicit, but ecpg neither needs nor has any such

PL/pgSQL also uses this script, so maybe just phrase it to exclude the
core keyword list.

-- 
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  36. Re: factorial function/phase out postfix operators?

    John Naylor <john.naylor@2ndquadrant.com> — 2020-08-24T07:28:17Z

    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

  37. Re: factorial function/phase out postfix operators?

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-08-24T15:04:00Z

    
> 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

  38. Re: factorial function/phase out postfix operators?

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-08-26T03:12:00Z

    
> 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 makes it harder for a person adding a new keyword to know what to place there.  If they guess "FALSE", and also don't know about adding the new keyword to the bare_label_keyword rule in gram.y, then those two mistakes will agree with each other and the person adding the keyword won't likely know they did it wrong.  It is simple enough for gen_keywordlist.pl to convert between what we use in kwlist.h and a boolean value for kwlist_d.h, so I did it that way.
> 
> Sounds fine to me.
> 
>>> In the bikeshedding department, it seems "implicit" was chosen because
>>> it was distinct from "bare". I think "bare" as a descriptor should be
>>> kept throughout for readability's sake. Maybe BareColLabel could be
>>> "IDENT or bare_label_keyword" for example. Same for the $status var.
>> 
>> The category "bare_label_keyword" is used in v3.  As for the $status var, I don't want to name that $bare, as I didn't go with your idea about using a boolean.  $status = "BARE_LABEL" vs "EXPLICIT_LABEL" makes sense to me, more than $bare = "BARE_LABEL" vs "EXPLICIT_LABEL" does.  "status" is still a bit vague, so more bikeshedding is welcome.
> 
> Yeah, it's very generic, but it's hard to find a short word for
> "can-be-used-as-a-bare-column-label-ness".

The construction colname AS colalias brings to mind the words "pseudonym" and "alias".  The distinction we're trying to draw here is between implicit pseudoyms and explicit ones, but "alias" is shorter and simpler, so I like that better than "pseudonym".  Both are labels, so adding "label" to the name doesn't really get us anything.  The constructions "implicit alias" vs. "explicit alias" seem to me to be an improvement, along with their other forms like "ImplicitAlias", or "implicit_alias", etc., so I've used those in version 4.

The word "status" here really means something like "plicity" (implict vs. explicit), but "plicity" isn't a word, so I used "aliastype" instead.

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 the new major version who have custom postfix operators will find that pg_upgrade chokes trying to recreate the postfix operator.  That's not great, but perhaps there is nothing automated that we could do for them that would be any better.
> 
> I'm thinking it would be good to have something like
> 
> select oid from pg_operator where oprright = 0 and oid >= FirstNormalObjectId;
> 
> in the pre-upgrade check.

Done.  Testing an upgrade of a 9.1 test install, relying on the regression database having left over user defined postfix operators, 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 there is 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 postfix operators 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 an alias, 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, it returns "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 not going to update it, who will?  The count may increase over time if other keywords are added, but I doubt anybody who adds 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.

> "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.

  39. Re: factorial function/phase out postfix operators?

    John Naylor <john.naylor@2ndquadrant.com> — 2020-08-26T13:33:20Z

    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 to draw here is between implicit pseudoyms and explicit ones, but "alias" is shorter and simpler, so I like that better than "pseudonym".  Both are labels, so adding "label" to the name doesn't really get us anything.  The constructions "implicit alias" vs. "explicit alias" seem to me to be an improvement, along with their other forms like "ImplicitAlias", or "implicit_alias", etc., so I've used those in version 4.

> The word "status" here really means something like "plicity" (implict vs. explicit), but "plicity" isn't a word, so I used "aliastype" instead.

Seems fine.

> A list of user defined postfix operators is in the file:
>     postfix_ops.txt
>
> Failure, exiting
>
>
> With the contents of postfix_ops.txt:
>
> In database: regression
>   (oid=27113) public.#@# (pg_catalog.int8)
>   (oid=27114) public.#%# (pg_catalog.int8)
>
> which should be enough for a user to identify which operator is meant.  I just invented that format.  Let me know if there is a preferred way to lay out that information.

Not sure if there's a precedent here, and seems fine to me.

+ /*
+ * If neither argument is specified, do not mention postfix operators, as
+ * the user is unlikely to have meant to create one.  It is more likely
+ * they simply neglected to mention the args.
+ */
  if (!OidIsValid(typeId1) && !OidIsValid(typeId2))
  ereport(ERROR,
  (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
- errmsg("at least one of leftarg or rightarg must be specified")));
+ errmsg("operator arguments must be specified")));
+
+ /*
+ * But if only the right arg is missing, they probably do intend to create
+ * a postfix operator, so give them a hint about why that does not work.
+ */
+ if (!OidIsValid(typeId2))
+ ereport(ERROR,
+ (errcode(ERRCODE_INVALID_FUNCTION_DEFINITION),
+ errmsg("operator right argument must be specified"),
+ errhint("Postfix operators are not supported.")));

This is just a nitpick -- I think the comments in this section would
flow better if order of checks were reversed, although the code might
not. I don't feel too strongly about it.

- * between POSTFIXOP and Op.  We can safely assign the same priority to
- * various unreserved keywords as needed to resolve ambiguities (this can't
- * have any bad effects since obviously the keywords will still behave the
- * same as if they weren't keywords).  We need to do this:
+ * greater than Op.  We can safely assign the same priority to various
+ * unreserved keywords as needed to resolve ambiguities (this can't have any
+ * bad effects since obviously the keywords will still behave the same as if
+ * they weren't keywords).  We need to do this:

I believe it's actually "lower than Op", and since POSTFIXOP is gone
it doesn't seem to matter how low it is. In fact, I found that the
lines with INDENT and UNBOUNDED now work as the lowest precedence
declarations. Maybe that's worth something?

Following on Peter E.'s example upthread, GENERATED can be removed
from precedence, and I also found the same is true for PRESERVE and
STRIP_P.

I've attached a patch which applies on top of 0001 to demonstrate
this. There might possibly still be syntax errors for things not
covered in the regression test, but there are no s/r conflicts at
least.

-{ oid => '389', descr => 'deprecated, use ! instead',
+{ oid => '389', descr => 'factorial',

Hmm, no objection, but it could be argued that we should just go ahead
and remove "!!" also, keeping only "factorial()". If we're going to
break a small amount of code using the normal math expression, it
seems silly to use a non-standard one that we deprecated before 2011
(cf. 908ab802864). On the other hand, removing it doesn't buy us
anything.

Some leftovers...

...in catalog/namespace.c:

OpernameGetOprid()
 * Pass oprleft = InvalidOid for a prefix op, oprright = InvalidOid for
 * a postfix op.

OpernameGetCandidates()
 * The returned items always have two args[] entries --- one or the other
 * will be InvalidOid for a prefix or postfix oprkind.  nargs is 2, too.

...in nodes/print.c:

/* we print prefix and postfix ops the same... */


> > 0002:
> >
> > + * All keywords can be used explicitly as a column label in expressions
> > + * like 'SELECT 1234 AS keyword', but only some keywords can be used
> > + * implicitly as column labels in expressions like 'SELECT 1234 keyword'.
> > + * Those that can be used implicitly should be listed here.
> >
> > In my mind, "AS" is the thing that's implied when not present, so we
> > should reword this to use the "bare" designation when talking about
> > the labels. I think there are contexts elsewhere where the implicit
> > column label is "col1, col2, col3...". I can't remember offhand where
> > that is though.
>
> Per my rambling above, I think what's really implied or explicit when "AS" is missing or present is that we're making an 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'm not going to update it, who will?  The count may increase over time if other keywords are added, but I doubt anybody who adds 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.

# 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

  40. Re: factorial function/phase out postfix operators?

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-08-26T15:57:53Z

    
> 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 following cleanup causes no apparent problems:

-%nonassoc      UNBOUNDED               /* ideally should have same precedence as IDENT */
-%nonassoc      IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
+%nonassoc      UNBOUNDED IDENT
+%nonassoc      PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP

Which does what the old comment apparently wanted.

> 
> -{ 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 warnings resolved 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 making an 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'm not going to update it, who will?  The count may increase over time if other keywords are added, but I doubt anybody who adds 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 number of 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 determine whether 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.

  41. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-08-26T17:55:26Z

    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 help determine whether the deprecation notices should go into v13?

Speaking of that, has somebody written a specific patch for that?
Like, exactly what are we proposing that this deprecation warning is
going to say?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  42. Re: factorial function/phase out postfix operators?

    John Naylor <john.naylor@2ndquadrant.com> — 2020-08-27T09:24:37Z

    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 following cleanup causes no apparent problems:
>
> -%nonassoc      UNBOUNDED               /* ideally should have same precedence as IDENT */
> -%nonassoc      IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
> +%nonassoc      UNBOUNDED IDENT
> +%nonassoc      PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
>
> Which does what the old comment apparently wanted.

This changes the context of the comment at the top of the block:

 * To support target_el without AS, we must give IDENT an explicit priority
 * lower than Op.  We can safely assign the same priority to various
 * unreserved keywords as needed to resolve ambiguities (this can't have any

This also works:

-%nonassoc      UNBOUNDED               /* ideally should have same
precedence as IDENT */
-%nonassoc      IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING
CUBE ROLLUP
+%nonassoc UNBOUNDED IDENT PARTITION RANGE ROWS GROUPS CUBE ROLLUP
+%nonassoc PRECEDING FOLLOWING

Not sure if either is better. Some additional input would be good here.

While looking for a place to put a v13 deprecation notice, I found
some more places in the docs which need updating:

ref/create_operator.sgml

"At least one of LEFTARG and RIGHTARG must be defined. For binary
operators, both must be defined. For right unary operators, only
LEFTARG should be defined, while for left unary operators only
RIGHTARG should be defined."

ref/create_opclass.sgml

"In an OPERATOR clause, the operand data type(s) of the operator, or
NONE to signify a left-unary or right-unary operator."

-- 
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  43. Re: factorial function/phase out postfix operators?

    John Naylor <john.naylor@2ndquadrant.com> — 2020-08-27T11:11:51Z

    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 help determine whether the deprecation notices should go into v13?
>
> Speaking of that, has somebody written a specific patch for that?
> Like, exactly what are we proposing that this deprecation warning is
> going to say?

Well, for starters it'll say the obvious, but since we have a concrete
timeframe, maybe a <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

  44. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-08-27T13:50:09Z

    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

  45. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-08-27T14:04:04Z

    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

  46. Re: factorial function/phase out postfix operators?

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-08-27T14:14:26Z

    
> 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 were choosing one operator per underlying proc to be the canonical operator name, and deprecating all other operators based on the same proc.  You chose postfix ! as the canonical operator for numeric_fac and deprecated prefix !!, but I think I can infer from that commit that if postfix ! did not exist, prefix !! would have been the canonical operator and would not have been deprecated.

The main reason I did not remove prefix !! in this patch series is that the patch is about removing postfix operator support, and so it seemed off topic.  But if there is general agreement to remove prefix !!, I'll put that in the next patch.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  47. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-08-27T14:19:21Z

    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

  48. Re: factorial function/phase out postfix operators?

    John Naylor <john.naylor@2ndquadrant.com> — 2020-08-28T08:43:58Z

    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

  49. Re: factorial function/phase out postfix operators?

    John Naylor <john.naylor@2ndquadrant.com> — 2020-08-28T08:44:54Z

    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 following cleanup causes no apparent problems:
>
> -%nonassoc      UNBOUNDED               /* ideally should have same precedence as IDENT */
> -%nonassoc      IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
> +%nonassoc      UNBOUNDED IDENT
> +%nonassoc      PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP

Thinking about this some more, I don't think we don't need to do any
precedence refactoring in order to apply the functional change of
these patches. We could leave that for follow-on patches once we
figure out the best way forward, which could take some time.

-- 
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  50. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-08-28T15:00:28Z

    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

  51. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-08-28T15:01:44Z

    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

  52. Re: factorial function/phase out postfix operators?

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-09-01T19:00:35Z

    
> 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 following cleanup causes no apparent problems:
>> 
>> -%nonassoc      UNBOUNDED               /* ideally should have same precedence as IDENT */
>> -%nonassoc      IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
>> +%nonassoc      UNBOUNDED IDENT
>> +%nonassoc      PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING CUBE ROLLUP
>> 
>> Which does what the old comment apparently wanted.
> 
> This changes the context of the comment at the top of the block:
> 
> * To support target_el without AS, we must give IDENT an explicit priority
> * lower than Op.  We can safely assign the same priority to various
> * unreserved keywords as needed to resolve ambiguities (this can't have any
> 
> This also works:
> 
> -%nonassoc      UNBOUNDED               /* ideally should have same
> precedence as IDENT */
> -%nonassoc      IDENT PARTITION RANGE ROWS GROUPS PRECEDING FOLLOWING
> CUBE ROLLUP
> +%nonassoc UNBOUNDED IDENT PARTITION RANGE ROWS GROUPS CUBE ROLLUP
> +%nonassoc PRECEDING FOLLOWING
> 
> Not sure if either is better. Some additional input would be good here.

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 can be 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 patch set is compatible with what was done there.  This patch set is intended for commit against master, targeted for PostgreSQL 14, so the deprecation notices are removed along with the things that were deprecated.  The references to right-unary operators that you call out, above, have been removed.

  53. Re: factorial function/phase out postfix operators?

    John Naylor <john.naylor@2ndquadrant.com> — 2020-09-02T08:33:57Z

    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 patch set is compatible with what was done there.  This patch set is intended for commit against master, targeted for PostgreSQL 14, so the deprecation notices are removed along with the things that were deprecated.  The references to right-unary operators that you call out, above, have been removed.

Hi Mark,

Looks good. Just a couple things I found in 0001:

The factorial operators should now be removed from func.sgml.

For pg_dump, should we issue a pg_log_warning() (or stronger)
somewhere if user-defined postfix operators are found? I'm looking at
the example of "WITH OIDS" in pg_dump.c.

Nitpick: these can be removed, since we already test factorial() in this file:

-SELECT 4!;
-SELECT !!3;
+SELECT factorial(4);
+SELECT factorial(3);

-- 
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  54. Re: factorial function/phase out postfix operators?

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-09-03T15:49:56Z

    
> 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 this patch set is compatible with what was done there.  This patch set is intended for commit against master, targeted for PostgreSQL 14, so the deprecation notices are removed along with the things that were deprecated.  The references to right-unary operators that you call out, above, have been removed.
> 
> Hi Mark,
> 
> Looks good. Just a couple things I found in 0001:
> 
> The factorial operators should now be removed from func.sgml.

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 back into an older server, I think doing anything stronger than a pg_log_warning() would be incorrect.  I did not find precedents under comparable circumstances for taking stronger actions than pg_log_warning.  I assume we can't, for example, omit the operator from the dump, nor can we abort the process.

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 removed in v7.

  55. Re: factorial function/phase out postfix operators?

    John Naylor <john.naylor@2ndquadrant.com> — 2020-09-07T15:43:00Z

    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

  56. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-09-11T15:06:26Z

    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 back into an older server, I think doing anything stronger than a pg_log_warning() would be incorrect.  I did not find precedents under comparable circumstances for taking stronger actions than pg_log_warning.  I assume we can't, for example, omit the operator from the dump, nor can we abort the process.

I'm not sure that this is the right solution. Generally, the
recommendation is that you should use the pg_dump that corresponds to
the server version where you want to do the reload, so if you're
hoping to dump 9.6 and restore on 11, you should be using the pg_dump
from 11, not 14. So my thought would be that if there are user-defined
postfix operators, pg_dump ought to error out. However, that could be
inconvenient for people who are using pg_dump in ways that are maybe
not what we would recommend but which may happen to work but for this
issue, so I'm not sure. On the third hand, though, we think that there
are very few user-defined postfix operators out there, so if we just
give an error, we probably won't be inconveniencing many people.

I'm not sure who is going to commit this work, and that person may
have a different preference than me. However, if it's me, I'd like to
see the removal of the existing postfix operators broken off into its
own patch, separate from the removal of the underlying facility to
have postfix operators.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  57. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-11T15:36:08Z

    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 back into an older server, I think doing anything stronger than a pg_log_warning() would be incorrect.  I did not find precedents under comparable circumstances for taking stronger actions than pg_log_warning.  I assume we can't, for example, omit the operator from the dump, nor can we abort the process.

> I'm not sure that this is the right solution. Generally, the
> recommendation is that you should use the pg_dump that corresponds to
> the server version where you want to do the reload, so if you're
> hoping to dump 9.6 and restore on 11, you should be using the pg_dump
> from 11, not 14. So my thought would be that if there are user-defined
> postfix operators, pg_dump ought to error out. However, that could be
> inconvenient for people who are using pg_dump in ways that are maybe
> not what we would recommend but which may happen to work but for this
> issue, so I'm not sure. On the third hand, though, we think that there
> are very few user-defined postfix operators out there, so if we just
> give an error, we probably won't be inconveniencing many people.

My inclination is to simply not change pg_dump.  There is no need to break
the use-case of loading the output back into the server version it came
from, if we don't have to.  If the output is getting loaded into a server
that lacks postfix operators, that server can throw the error.  There's no
real gain in having pg_dump prejudge the issue.

			regards, tom lane

  58. Re: factorial function/phase out postfix operators?

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-09-11T16:39:36Z

    
> 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 back into an older server, I think doing anything stronger than a pg_log_warning() would be incorrect.  I did not find precedents under comparable circumstances for taking stronger actions than pg_log_warning.  I assume we can't, for example, omit the operator from the dump, nor can we abort the process.
> 
>> I'm not sure that this is the right solution. Generally, the
>> recommendation is that you should use the pg_dump that corresponds to
>> the server version where you want to do the reload, so if you're
>> hoping to dump 9.6 and restore on 11, you should be using the pg_dump
>> from 11, not 14. So my thought would be that if there are user-defined
>> postfix operators, pg_dump ought to error out. However, that could be
>> inconvenient for people who are using pg_dump in ways that are maybe
>> not what we would recommend but which may happen to work but for this
>> issue, so I'm not sure. On the third hand, though, we think that there
>> are very few user-defined postfix operators out there, so if we just
>> give an error, we probably won't be inconveniencing many people.
> 
> My inclination is to simply not change pg_dump.  There is no need to break
> the use-case of loading the output back into the server version it came
> from, if we don't have to.  If the output is getting loaded into a server
> that lacks postfix operators, that server can throw the error.  There's no
> real gain in having pg_dump prejudge the issue.

I think some kind of indication that the dump won't be loadable is useful if they're planning to move the dump file across an expensive link, or if they intend to blow away the old data directory to make room for the new.  Whether that indication should be in the form of a warning or an error is less clear to me.  Whatever we do here, I think it sets a precedent for how such situations are handled in the future, so maybe focusing overmuch on the postfix operator issue is less helpful than on the broader concept.  What, for example, would we do if we someday dropped GiST support?  Print a warning when dumping a database with GiST indexes?  Omit the indexes?  Abort the dump?

The docs at https://www.postgresql.org/docs/12/app-pgdump.html say:

> Because pg_dump is used to transfer data to newer versions of PostgreSQL, the output of pg_dump can be expected to load into PostgreSQL server versions newer than pg_dump's version.
<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 the dump was taken from a server of that version.


I think somewhere around here the docs need to call out what happens when the older major version supported a feature that has been dropped from the newer major version.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  59. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-11T18:25:27Z

    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

  60. Re: factorial function/phase out postfix operators?

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-09-11T19:23:17Z

    
> 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 to do with postfix operators specifically, but be more general like --dump-incompatible-objects vs. --omit-incompatible-objects vs. --error-on-incompatible-objects vs. --do-your-best-to-fixup-incompatible-objects, with one of those being the default (and with all of them having better names).  If --error-on-incompatible-objects were the default, that would behave as Robert recommended upthread.

I can totally see an objection to the added complexity of such options, so I'm really just putting this out on the list for comment.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  61. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-09-11T19:54:28Z

    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 to do with postfix operators specifically, but be more general like --dump-incompatible-objects vs. --omit-incompatible-objects vs. --error-on-incompatible-objects vs. --do-your-best-to-fixup-incompatible-objects, with one of those being the default (and with all of them having better names).  If --error-on-incompatible-objects were the default, that would behave as Robert recommended upthread.
>
> I can totally see an objection to the added complexity of such options, so I'm really just putting this out on the list for comment.

I'm not opposed to Tom's proposal. I just wanted to raise the issue
for discussion.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  62. Re: factorial function/phase out postfix operators?

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-09-11T19:56:59Z

    
> 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 anything to do with postfix operators specifically, but be more general like --dump-incompatible-objects vs. --omit-incompatible-objects vs. --error-on-incompatible-objects vs. --do-your-best-to-fixup-incompatible-objects, with one of those being the default (and with all of them having better names).  If --error-on-incompatible-objects were the default, that would behave as Robert recommended upthread.
>> 
>> I can totally see an objection to the added complexity of such options, so I'm really just putting this out on the list for comment.
> 
> I'm not opposed to Tom's proposal. I just wanted to raise the issue
> for discussion.

Ah, ok.  I don't feel any need for changes, either.  I'll leave the patch as it stands now.

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  63. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-11T20:51:50Z

    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 anything to do with postfix operators specifically, but be more general like --dump-incompatible-objects vs. --omit-incompatible-objects vs. --error-on-incompatible-objects vs. --do-your-best-to-fixup-incompatible-objects, with one of those being the default (and with all of them having better names).  If --error-on-incompatible-objects were the default, that would behave as Robert recommended upthread.
>>> I can totally see an objection to the added complexity of such options, so I'm really just putting this out on the list for comment.

>> I'm not opposed to Tom's proposal. I just wanted to raise the issue
>> for discussion.

> Ah, ok.  I don't feel any need for changes, either.  I'll leave the
> patch as it stands now.

We're in violent agreement it seems.

At some point it might be worth doing something like what Mark suggests
above, but this patch shouldn't be tasked with it.  In any case, since
pg_dump does not know what the target server version really is, it's
going to be hard for it to authoritatively distinguish what will work
or not.

			regards, tom lane

  64. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-17T20:19:05Z

    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

  65. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-18T15:29:39Z

    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

  66. Re: factorial function/phase out postfix operators?

    Mark Dilger <mark.dilger@enterprisedb.com> — 2020-09-18T15:43:52Z

    
> 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

  67. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-09-18T17:42:53Z

    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

  68. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-18T18:11:37Z

    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

  69. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-09-18T18:34:27Z

    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

  70. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-18T19:31:18Z

    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

  71. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-18T20:48:25Z

    Pushed with the discussed terminological changes and some other
fooling about, including fixing the documentation.

			regards, tom lane

  72. Re: factorial function/phase out postfix operators?

    Robert Haas <robertmhaas@gmail.com> — 2020-09-19T00:18:16Z

    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

  73. Re: factorial function/phase out postfix operators?

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-09-19T19:20:57Z

    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