Date: Sun, 3 Jul 2022 01:01:27 -0700
From: Noah Misch <noah@leadboat.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Meskes <meskes@postgresql.org>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: SQL/JSON functions vs. ECPG vs. STRING as a reserved word
Message-ID: <20220703080127.GA2476530@rfd.leadboat.com>
References: <3661437.1653855582@sss.pgh.pa.us>
 <YpTFvHBWT4OlLqCP@feivel.guest.credativ.lan>
 <4158666.1653945615@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <4158666.1653945615@sss.pgh.pa.us>
User-Agent: Mutt/1.5.24 (2015-08-30)
Archived-At: 
 <https://www.postgresql.org/message-id/20220703080127.GA2476530%40rfd.leadboat.com>
Precedence: bulk

On Mon, May 30, 2022 at 05:20:15PM -0400, Tom Lane wrote:

[allow EXEC SQL TYPE unreserved_keyword IS ...]

> 1. In pgc.l, if an identifier is a typedef name, ignore any possible
> keyword meaning and return it as an IDENT.  (I'd originally supposed
> that we'd want to return some new TYPEDEF token type, but that does
> not seem to be necessary right now, and adding a new token type would
> increase the patch footprint quite a bit.)
> 
> 2. In the var_type production, forget about ECPGColLabel[Common]
> and just handle the keywords we know we need, plus IDENT for the
> typedef case.  It turns out that we have to have duplicate coding
> because most of these keywords are not keywords in C lexing mode,
> so that they'll come through the IDENT path anyway when we're
> in a C rather than SQL context.  That seemed acceptable to me.
> I thought about adding them all to the C keywords list but that
> seemed likely to have undesirable side-effects, and again it'd
> bloat the patch footprint.
> 
> This fix is not without downsides.  Disabling recognition of
> keywords that match typedefs means that, for example, if you
> declare a typedef named "work" then ECPG will fail to parse
> "EXEC SQL BEGIN WORK".  So in a real sense this is just trading
> one hazard for another.  But there is an important difference:
> with this, whether your ECPG program works depends only on what
> typedef names and SQL commands are used in the program.  If
> it compiles today it'll still compile next year, whereas with
> the present implementation the addition of some new unreserved
> SQL keyword could break it.  We'd have to document this change
> for sure, and it wouldn't be something to back-patch, but it
> seems like it might be acceptable from the users' standpoint.

I agree this change is more likely to please a user than to harm a user.  The
user benefit is slim, but the patch is also slim.

> We could narrow (not eliminate) this hazard if we could get the
> typedef lookup in pgc.l to happen only when we're about to parse
> a var_type construct.  But because of Bison's lookahead behavior,
> that seems to be impossible, or at least undesirably messy
> and fragile.  But perhaps somebody else will see a way.

I don't, though I'm not much of a Bison wizard.

> Anyway, this seems like too big a change to consider for v15,
> so I'll stick this patch into the v16 CF queue.  It's only
> draft quality anyway --- lacks documentation changes and test
> cases.  There are also some coding points that could use review.
> Notably, I made the typedef lookup override SQL keywords but
> not C keywords; this is for consistency with the C-mode lookup
> rules, but is it the right thing?

That decision seems fine.  ScanCKeywordLookup() covers just twenty-six
keywords, and that list hasn't changed since 2003.  Moreover, most of them are
keywords of the C language itself, so allowing them would entailing mangling
them in the generated C to avoid C compiler errors.  Given the lack of
complaints, let's not go there.

I didn't locate any problems beyond the test and doc gaps that you mentioned,
so I've marked this Ready for Committer.