On Thu, Jul 21, 2022 at 07:42:12PM +0200, Alvaro Herrera wrote:
> Thanks. I was looking at the recently modified REINDEX syntax and
> noticed there another spot for taking an optional name. I ended up
> reusing OptSchemaName for that, as in the attached patch. I think
> adding production-specific additional productions is pointless and
> probably bloats the grammar. So let me +1 your push of the patch you
> posted, just to keep things moving forward, but in addition I propose to
> later rename OptSchemaName to something more generic and use it in these
> three places.
This slightly changes the behavior of the grammar, as CONCURRENTLY
was working on DATABASE as follows:
* On HEAD:
=# reindex database concurrently postgres;
REINDEX
=# reindex (concurrently) database concurrently postgres;
REINDEX
=# reindex (concurrently) database ;
REINDEX
=# reindex (concurrently) database postgres;
REINDEX
=# reindex database concurrently postgres;
REINDEX
=# reindex database concurrently;
ERROR: 42601: syntax error at or near ";"
And actually, even on HEAD, the last case is marked as supported by
the docs but we don't allow it in the parser. My mistake on this
one.
Now, with the patch, I get:
=# reindex (concurrently) database concurrently;
ERROR: 42601: syntax error at or near "concurrently"
LINE 1: reindex (concurrently) database concurrently postgres ;
=# reindex (concurrently) database concurrently postgres;
ERROR: 42601: syntax error at or near "concurrently"
LINE 1: reindex (concurrently) database concurrently postgres;
=# reindex (concurrently) database ;
REINDEX
=# reindex (concurrently) database postgres;
REINDEX
=# reindex database concurrently postgres;
ERROR: 42601: syntax error at or near "concurrently"
LINE 1: reindex database concurrently postgres;
=# reindex database concurrently;
ERROR: 42601: syntax error at or near "concurrently"
So this indeed has as effect to make possible the use of CONCURRENTLY
for DATABASE and SYSTEM only within the parenthesized grammar. Seeing
the simplifications this creates, I'd agree with dropping this part of
the grammar. I think that I would add the following queries to
create_index.sql to test this grammar, as we can rely on this code
path generating an error:
REINDEX (CONCURRENTLY) SYSTEM postgres;
REINDEX (CONCURRENTLY) SYSTEM;
At least it would validate the parsing for DATABASE.
This breaks reindexdb for the database case, because the query
generated in run_reindex_command() adds CONCURRENTLY only *after* the
database name, and we should be careful to generate something
backward-compatible in this tool, as well. The fix is simple: you
can add CONCURRENTLY within the section with TABLESPACE and VERBOSE
for a connection >= 14, and use it after the object name for <= 13.
--
Michael