Re: psql command aliases support

Bernd Helmle <mailings(at)oopsware(dot)de> writes:
> please find attached a patch which implements psql command aliases. They
> work the same way as on bash, zsh and others, for example:

I'm still not convinced that we want this sort of feature at all.
I quote from the current bash manual page:

ALIASES
...
The rules concerning the definition and use of aliases are somewhat
confusing.
...
For almost every purpose, aliases are superseded by shell functions.

The bash authors seem to wish they'd never adopted the feature, which
makes me wonder whether we really want to copy it slavishly.

Having said that, though, here are some code-review points:

* The patch intends that the expansion of an alias could be either a
backslash command or SQL command text, but I don't think you've thought
through the interactions of these cases. In particular, the patch doesn't
seem to behave very sanely if the backslash command comes when there is
already text on the current line or previous lines of the query buffer:
it just wipes that text out, which is surely not the desirable thing.

* Use of a VariableSpace for the stack of aliases-being-expanded seems
exceedingly klugy: variable spaces are intended for persistent storage
not transient state, so this fails to satisfy the principle of least
astonishment for readers of the code. Also, you're failing to manipulate
it as a stack: you shouldn't wipe a stack entry upon matching it, only
when control returns from having expanded it. Another problem is that
it doesn't get reset at all if the alias expansion is just SQL text and
not another backslash command, meaning successive uses of such an alias
won't work. I think you'd be better off if the expansion were done as a
separate recursive routine with the set of active aliases just passed as
a list threaded through local variables of the routine invocations.

* I think you should lose the separate PSQL_CMD_ALIAS status code and
just use PSQL_CMD_NEWEDIT. The one place where the codes act different
looks like a mistake to me anyway.

* Don't feed non-constant strings through gettext():

+ if (cmd)
+ {
+ puts(_(cmd));
+ success = true;
+ }

The best you can hope for is that it doesn't do anything.

regards, tom lane