On Fri, Jul 22, 2022 at 03:06:46PM +0200, Alvaro Herrera wrote:
> Actually, looking at the grammar again I realized that the '('options')'
> part could be refactored; and with that, keeping an extra production for
> REINDEX DATABASE CONCURRENTLY is short enough. It is removed from
> REINDEX SYSTEM, but that's OK because that doesn't work anyway.
I have just looked at 83011ce, and got what you've done here. You
have thrown away reindex_target_multitable and added three parts for
SCHEMA, DATABASE and SYSTEM instead with their own options, enforcing
the restriction on CONCURRENTLY at the end of REINDEX SYSTEM in the
parser rather than indexcmds.c.
+ | REINDEX opt_reindex_option_list SYSTEM_P opt_single_name
{
ReindexStmt *n =
makeNode(ReindexStmt);
-
- n->kind = $5;
- n->name = $7;
+ n->kind = REINDEX_OBJECT_SYSTEM;
+ n->name = NULL;
I think that there is a bug in this logic. ReindexStmt->name is
always set to NULL, meaning that a REINDEX command with any database
name would still pass, but I don't think that we should allow that.
For example, with something like these commands, we should complain
that the database name specified does not match with the database we
are connected to:
=# reindex system popo_foo_bar;
REINDEX
=# reindex database popo_foo_bar;
REINDEX
It may have been better to wait a bit if you wanted me to look at all
that, as our timezones are not compatible, especially on Fridays, but
that's fine :D.
--
Michael