Re: some grammar refactoring

> On May 18, 2020, at 11:43 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>
> Here is a series of patches to do some refactoring in the grammar around the commands COMMENT, DROP, SECURITY LABEL,
andALTER EXTENSION ... ADD/DROP.  In the grammar, these commands (with some exceptions) basically just take a reference
toan object and later look it up in C code.  Some of that was already generalized individually for each command
(drop_type_any_name,drop_type_name, etc.).  This patch combines it into common lists for all these commands. 
>
> Advantages:
>
> - Avoids having to list each object type at least four times.
>
> - Object types not supported by security labels or extensions are now explicitly listed and give a proper error
message. Previously, this was just encoded in the grammar itself and specifying a non-supported object type would just
givea parse error. 
>
> - Reduces lines of code in gram.y.
>
> - Removes some old cruft.

I like the general direction you are going with this, but the decision in v1-0006 to move the error for invalid object
typesout of gram.y and into extension.c raises an organizational question.   At some places in gram.y, there is C code
thatchecks parsed tokens and ereports if they are invalid, in some sense extending the grammar right within gram.y.  In
manyother places, including what you are doing in this patch, the token is merely stored in a Stmt object with the
errorchecking delayed until command processing.  For tokens which need to be checked against the catalogs, that
decisionmakes perfect sense.  But for ones where all the information necessary to validate the token exists in the
parser,it is not clear to me why it gets delayed until command processing.  Is there a design principle behind when
thesechecks are done in gram.y vs. when they are delayed to the command processing?  I'm guessing in v1-0006 that you
aredoing it this way because there are multiple places in gram.y where tokens would need to be checked, and by delaying
thecheck until ExecAlterExtensionContentsStmt, you can put the check all in one place.  Is that all it is? 

I have had reason in the past to want to reorganize gram.y to have all these types of checks in a single, consistent
formatand location, rather than scattered through gram.y and backend/commands/.  Does anybody else have an interest in
this?

My interest in this stems from the fact that bison can be run to generate data files that can then be used in reverse
togenerate random SQL.  The more the parsing logic is visible to bison, the more useful the generated data files are.
Buta single, consistent design for extra-grammatical error checks could help augment those files fairly well, too. 

—
Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company