From: Noah Misch <noah@leadboat.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Michael Meskes <meskes@postgresql.org>
Cc: pgsql-hackers@lists.postgresql.org
Subject: Re: SQL/JSON functions vs. ECPG vs. STRING as a reserved word
Date: Sun, 3 Jul 2022 01:01:27 -0700
Message-ID: <20220703080127.GA2476530@rfd.leadboat.com> (raw)
In-Reply-To: <4158666.1653945615@sss.pgh.pa.us>
References: <3661437.1653855582@sss.pgh.pa.us>
<YpTFvHBWT4OlLqCP@feivel.guest.credativ.lan>
<4158666.1653945615@sss.pgh.pa.us>
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.
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: noah@leadboat.com, tgl@sss.pgh.pa.us, meskes@postgresql.org, pgsql-hackers@lists.postgresql.org
Subject: Re: SQL/JSON functions vs. ECPG vs. STRING as a reserved word
In-Reply-To: <20220703080127.GA2476530@rfd.leadboat.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox