Re: factorial function/phase out postfix operators?

On Wed, Aug 26, 2020 at 6:12 AM Mark Dilger
<mark(dot)dilger(at)enterprisedb(dot)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(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.

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