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
signature.asc
Description: PGP signature