BUG #15555: Syntax errors when using the COMMENT command in plpgsql and a "comment" variable

The following bug has been logged on the website:

Bug reference: 15555
Logged by: Feike Steenbergen
Email address: feikesteenbergen(at)gmail(dot)com
PostgreSQL version: 11.1
Operating system: CentOS Linux release 7.5.1804 (Core)
Description:

Recently, I ran into an issue when trying to put comments on objects in
plpgsql DO block, for example the following:

DO $$
DECLARE
"comment" text := 'This is a comment';
BEGIN
COMMENT ON TABLE abc IS 'This is another comment';
END;
$$;

Generates the following error message:

ERROR: 42601: syntax error at or near "ON"
LINE 5: COMMENT ON TABLE abc IS 'This is another comment';

Renaming the variable from "comment" to something else makes the problem go
away,
as well as wrapping the COMMENT command in an EXECUTE call;
It happens both when double quoting the identifier, or when omitting the
double
quotes.

For now I'll just avoid using comment as a variable name altogether, but I
was
surprised by this behaviour.

Kind regards,

Feike

=?utf-8?q?PG_Bug_reporting_form?= <noreply(at)postgresql(dot)org> writes:
> Recently, I ran into an issue when trying to put comments on objects in
> plpgsql DO block, for example the following:
>
> DO $$
> DECLARE
> "comment" text := 'This is a comment';
> BEGIN
> COMMENT ON TABLE abc IS 'This is another comment';
> END;
> $$;
>
> Generates the following error message:
> ERROR: 42601: syntax error at or near "ON"
> LINE 5: COMMENT ON TABLE abc IS 'This is another comment';
>
> Renaming the variable from "comment" to something else makes the problem go
> away,

This isn't specific to COMMENT, you can cause problems with variables
named like any statement-introducing keyword. For instance

regression=# do $$
declare update int;
begin
update foo set bar = 42;
end$$;
ERROR: syntax error at or near "foo"
LINE 4: update foo set bar = 42;
^

On the other hand, such a variable works fine as long as you use it
as a variable:

regression=# do $$
declare update int;
begin
update := 42;
raise notice 'update = %', update;
end$$;
NOTICE: update = 42
DO

One idea is to get rid of the ambiguity by making all such words reserved
so far as plpgsql is concerned, but nobody would thank us for that.
It would be a maintenance problem too, because the plpgsql parser doesn't
otherwise have a list of such keywords.

I wonder whether we could improve matters by adjusting the heuristic for
such things in pl_scanner.c:

* If we are at start of statement, prefer unreserved keywords
* over variable names, unless the next token is assignment or
* '[', in which case prefer variable names. (Note we need not
* consider '.' as the next token; that case was handled above,
* and we always prefer variable names in that case.) If we are
* not at start of statement, always prefer variable names over
* unreserved keywords.

The trouble with special-casing unreserved keywords here is precisely
that those only include words that are special to plpgsql, not words
that introduce statements of the main SQL grammar.

Maybe, if we are at start of statement, we should not even consider the
possibility of matching an unqualified identifier to a variable name
unless the next token is assignment or '['. IOW, the logic here would
become (1) if not at statement start, or if next token is assignment or
'[', see if the identifier matches a variable name. (2) If not, see if
it matches an unreserved plpgsql keyword. (3) If not, assume it is a SQL
keyword.

Are there any other cases where recognizing a variable name at statement
start is the correct thing to do? Even if it's not correct, could this
result in worse error messages? I think you probably just end up with
"syntax error" whenever we guess wrong, but I might be missing something.

regards, tom lane

I wrote:
> I wonder whether we could improve matters by adjusting the heuristic for
> such things in pl_scanner.c:
>
> * If we are at start of statement, prefer unreserved keywords
> * over variable names, unless the next token is assignment or
> * '[', in which case prefer variable names. (Note we need not
> * consider '.' as the next token; that case was handled above,
> * and we always prefer variable names in that case.) If we are
> * not at start of statement, always prefer variable names over
> * unreserved keywords.

Digging in the commit history, that code all dates to commit bb1b8f69,
which arose from a discussion about making plpgsql keywords unreserved:

https://www.postgresql.org/message-id/5030.1416778830%40sss.pgh.pa.us

AFAICS, the question of conflicts against core-parser keywords didn't
come up at all in that thread, so it's not surprising we didn't consider
that case. I don't see any indication that we explicitly rejected
doing something for that case.

There's still the question of whether there are any cases where we
*need* to recognize a variable name that's at statement start and
isn't followed by assignment or '['.

regards, tom lane

I wrote:
>> I wonder whether we could improve matters by adjusting the heuristic for
>> such things in pl_scanner.c:
>>
>> * If we are at start of statement, prefer unreserved keywords
>> * over variable names, unless the next token is assignment or
>> * '[', in which case prefer variable names. (Note we need not
>> * consider '.' as the next token; that case was handled above,
>> * and we always prefer variable names in that case.) If we are
>> * not at start of statement, always prefer variable names over
>> * unreserved keywords.

[ pokes at that for a bit ] The logic here is a bit denser than
one could wish, but here's a draft patch that seems to get the
job done. It passes check-world, which isn't conclusive but
at least suggests that this doesn't break anything.

I'll add this to the next CF in hopes that somebody will try to
break it.

regards, tom lane

On 17/12/2018 23:05, Tom Lane wrote:
> [ pokes at that for a bit ] The logic here is a bit denser than
> one could wish, but here's a draft patch that seems to get the
> job done. It passes check-world, which isn't conclusive but
> at least suggests that this doesn't break anything.

This looks good to me. I can't think of any other syntax scenarios that
it needs to address.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Peter Eisentraut <peter(dot)eisentraut(at)2ndquadrant(dot)com> writes:
> On 17/12/2018 23:05, Tom Lane wrote:
>> [ pokes at that for a bit ] The logic here is a bit denser than
>> one could wish, but here's a draft patch that seems to get the
>> job done. It passes check-world, which isn't conclusive but
>> at least suggests that this doesn't break anything.

> This looks good to me. I can't think of any other syntax scenarios that
> it needs to address.

Pushed, thanks for checking!

regards, tom lane