Re: factorial function/phase out postfix operators?

> On Jul 29, 2020, at 5:03 AM, John Naylor <john(dot)naylor(at)2ndquadrant(dot)com> wrote:
>
> 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.

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(dot)#(at)# (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.