Re: factorial function/phase out postfix operators?

On Wed, Jul 22, 2020 at 8:47 AM Mark Dilger
<mark(dot)dilger(at)enterprisedb(dot)com> wrote:
>
>
>
> > On Jul 18, 2020, at 1:00 AM, John Naylor <john(dot)naylor(at)2ndquadrant(dot)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