Re: some grammar refactoring - Mailing list pgsql-hackers

> On May 25, 2020, at 2:55 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
>
> On 2020-05-22 18:53, Mark Dilger wrote:
>> I like the general direction you are going with this, but the decision in v1-0006 to move the error for invalid
objecttypes out of gram.y and into extension.c raises an organizational question.   At some places in gram.y, there is
Ccode that checks parsed tokens and ereports if they are invalid, in some sense extending the grammar right within
gram.y. In many other places, including what you are doing in this patch, the token is merely stored in a Stmt object
withthe error checking delayed until command processing.  For tokens which need to be checked against the catalogs,
thatdecision makes 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? 
>
> We have been for some time moving to a style where we rely on switch statements around OBJECT_* constants to (a)
decidewhat is allowed with certain object types, and (b) make sure we have an explicit decision on each object type and
don'tforget any.  This has worked well, I think. 

Yes, I think so, too.  I like that overall design.

> This is more of that.

Yes, it is.

> Before this patch, it would have been pretty hard to find out which object types are supported with extensions or
securitylabels, except by very carefully reading the grammar. 

Fair enough.

> Moreover, you now get a proper error message for unsupported object types rather than just a generic parse error.

Sounds great.

>> 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
reverseto generate random SQL.  The more the parsing logic is visible to bison, the more useful the generated data
filesare.  But a single, consistent design for extra-grammatical error checks could help augment those files fairly
well,too. 
>
> It's certainly already the case that the grammar accepts statements that end up being invalid, even if you ignore
cataloglookup.  I don't think my patch moves the needle on this in a significant way. 

I don't think it moves the needle too much, either.  But since your patch is entirely a refactoring patch and not a
featurepatch, I thought it would be fair to ask larger questions about how the code should be structured.  I like using
enumsand switch statements and getting better error messages, but there doesn't seem to be any fundamental reason why
thatshould be in the command execution step.  It feels like a layering violation to me. 

I don't object to this patch getting committed.  A subsequent patch to consolidate all the grammar checks into
src/backend/parserand out of src/backend/commands won't be blocked by this. 

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