Re: factorial function/phase out postfix operators?

> On Aug 26, 2020, at 6:33 AM, John Naylor <john(dot)naylor(at)2ndquadrant(dot)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.