Location via proxy:   [ UP ]  
[Report a bug]   [Manage cookies]                
Skip to content

Commit 0a5f06b

Browse files
committed
Fix a few issues with REINDEX grammar
This addresses a couple of bugs in the REINDEX grammar, introduced by 83011ce: - A name was never specified for DATABASE/SYSTEM, even if the query included one. This caused such REINDEX queries to always work with any object name, but we should complain if the object name specified does not match the name of the database we are connected to. A test is added for this case in the main regression test suite, provided by Álvaro. - REINDEX SYSTEM CONCURRENTLY [name] was getting rejected in the parser. Concurrent rebuilds are not supported for catalogs but the error provided at execution time is more helpful for the user, and allowing this flavor results in a simplification of the parsing logic. - REINDEX DATABASE CONCURRENTLY was rebuilding the index in a non-concurrent way, as the option was not being appended correctly in the list of DefElems in ReindexStmt (REINDEX (CONCURRENTLY) DATABASE was working fine. A test is added in the TAP tests of reindexdb for this case, where we already have a REINDEX DATABASE CONCURRENTLY query running on a small-ish instance. This relies on the work done in 2cbc3c1 for SYSTEM, but here we check if the OIDs of the index relations match or not after the concurrent rebuild. Note that in order to get this part to work, I had to tweak the tests so as the index OID and names are saved separately. This change not affect the reliability or of the coverage of the existing tests. While on it, I have implemented a tweak in the grammar to reduce the parsing by one branch, simplifying things even more. Author: Michael Paquier, Álvaro Herrera Discussion: https://postgr.es/m/YttqI6O64wDxGn0K@paquier.xyz
1 parent 7c34555 commit 0a5f06b

File tree

5 files changed

+49
-36
lines changed

5 files changed

+49
-36
lines changed

doc/src/sgml/ref/reindex.sgml

+1-2
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,7 @@ PostgreSQL documentation
2222
<refsynopsisdiv>
2323
<synopsis>
2424
REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { INDEX | TABLE | SCHEMA } [ CONCURRENTLY ] <replaceable class="parameter">name</replaceable>
25-
REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] DATABASE [ CONCURRENTLY ] [ <replaceable class="parameter">name</replaceable> ]
26-
REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] SYSTEM [ <replaceable class="parameter">name</replaceable> ]
25+
REINDEX [ ( <replaceable class="parameter">option</replaceable> [, ...] ) ] { DATABASE | SYSTEM } [ CONCURRENTLY ] [ <replaceable class="parameter">name</replaceable> ]
2726

2827
<phrase>where <replaceable class="parameter">option</replaceable> can be one of:</phrase>
2928

src/backend/parser/gram.y

+17-19
Original file line numberDiff line numberDiff line change
@@ -564,7 +564,7 @@ static Node *makeRecursiveViewSelect(char *relname, List *aliases, Node *query);
564564
%type <defelt> generic_option_elem alter_generic_option_elem
565565
%type <list> generic_option_list alter_generic_option_list
566566

567-
%type <ival> reindex_target_type
567+
%type <ival> reindex_target_relation reindex_target_all
568568
%type <list> opt_reindex_option_list
569569

570570
%type <node> copy_generic_opt_arg copy_generic_opt_arg_list_item
@@ -9092,13 +9092,12 @@ DropTransformStmt: DROP TRANSFORM opt_if_exists FOR Typename LANGUAGE name opt_d
90929092
*
90939093
* QUERY:
90949094
*
9095-
* REINDEX [ (options) ] {TABLE | INDEX | SCHEMA} [CONCURRENTLY] <name>
9096-
* REINDEX [ (options) ] DATABASE [CONCURRENTLY] [<name>]
9097-
* REINDEX [ (options) ] SYSTEM [<name>]
9095+
* REINDEX [ (options) ] {INDEX | TABLE | SCHEMA} [CONCURRENTLY] <name>
9096+
* REINDEX [ (options) ] {DATABASE | SYSTEM} [CONCURRENTLY] [<name>]
90989097
*****************************************************************************/
90999098

91009099
ReindexStmt:
9101-
REINDEX opt_reindex_option_list reindex_target_type opt_concurrently qualified_name
9100+
REINDEX opt_reindex_option_list reindex_target_relation opt_concurrently qualified_name
91029101
{
91039102
ReindexStmt *n = makeNode(ReindexStmt);
91049103

@@ -9116,37 +9115,36 @@ ReindexStmt:
91169115
ReindexStmt *n = makeNode(ReindexStmt);
91179116

91189117
n->kind = REINDEX_OBJECT_SCHEMA;
9119-
n->name = $5;
91209118
n->relation = NULL;
9119+
n->name = $5;
91219120
n->params = $2;
91229121
if ($4)
91239122
n->params = lappend(n->params,
91249123
makeDefElem("concurrently", NULL, @4));
91259124
$$ = (Node *) n;
91269125
}
9127-
| REINDEX opt_reindex_option_list DATABASE opt_concurrently opt_single_name
9128-
{
9129-
ReindexStmt *n = makeNode(ReindexStmt);
9130-
n->kind = REINDEX_OBJECT_DATABASE;
9131-
n->name = NULL;
9132-
n->relation = NULL;
9133-
n->params = $2;
9134-
$$ = (Node *) n;
9135-
}
9136-
| REINDEX opt_reindex_option_list SYSTEM_P opt_single_name
9126+
| REINDEX opt_reindex_option_list reindex_target_all opt_concurrently opt_single_name
91379127
{
91389128
ReindexStmt *n = makeNode(ReindexStmt);
9139-
n->kind = REINDEX_OBJECT_SYSTEM;
9140-
n->name = NULL;
9129+
9130+
n->kind = $3;
91419131
n->relation = NULL;
9132+
n->name = $5;
91429133
n->params = $2;
9134+
if ($4)
9135+
n->params = lappend(n->params,
9136+
makeDefElem("concurrently", NULL, @4));
91439137
$$ = (Node *) n;
91449138
}
91459139
;
9146-
reindex_target_type:
9140+
reindex_target_relation:
91479141
INDEX { $$ = REINDEX_OBJECT_INDEX; }
91489142
| TABLE { $$ = REINDEX_OBJECT_TABLE; }
91499143
;
9144+
reindex_target_all:
9145+
SYSTEM_P { $$ = REINDEX_OBJECT_SYSTEM; }
9146+
| DATABASE { $$ = REINDEX_OBJECT_DATABASE; }
9147+
;
91509148
opt_reindex_option_list:
91519149
'(' utility_option_list ')' { $$ = $2; }
91529150
| /* EMPTY */ { $$ = NULL; }

src/bin/scripts/t/090_reindexdb.pl

+25-12
Original file line numberDiff line numberDiff line change
@@ -40,20 +40,20 @@
4040
# REINDEX operations. A set of relfilenodes is saved from the catalogs
4141
# and then compared with pg_class.
4242
$node->safe_psql('postgres',
43-
'CREATE TABLE index_relfilenodes (parent regclass, indname regclass, relfilenode oid);'
43+
'CREATE TABLE index_relfilenodes (parent regclass, indname text, indoid oid, relfilenode oid);'
4444
);
4545
# Save the relfilenode of a set of toast indexes, one from the catalog
4646
# pg_constraint and one from the test table.
4747
my $fetch_toast_relfilenodes =
48-
qq{SELECT b.oid::regclass, c.oid::regclass, c.relfilenode
48+
qq{SELECT b.oid::regclass, c.oid::regclass::text, c.oid, c.relfilenode
4949
FROM pg_class a
5050
JOIN pg_class b ON (a.oid = b.reltoastrelid)
5151
JOIN pg_index i on (a.oid = i.indrelid)
5252
JOIN pg_class c on (i.indexrelid = c.oid)
5353
WHERE b.oid IN ('pg_constraint'::regclass, 'test1'::regclass)};
5454
# Same for relfilenodes of normal indexes. This saves the relfilenode
5555
# from an index of pg_constraint, and from the index of the test table.
56-
my $fetch_index_relfilenodes = qq{SELECT i.indrelid, a.oid, a.relfilenode
56+
my $fetch_index_relfilenodes = qq{SELECT i.indrelid, a.oid::regclass::text, a.oid, a.relfilenode
5757
FROM pg_class a
5858
JOIN pg_index i ON (i.indexrelid = a.oid)
5959
WHERE a.relname IN ('pg_constraint_oid_index', 'test1x')};
@@ -69,6 +69,8 @@
6969
# parent table is included to provide more context.
7070
my $compare_relfilenodes = qq(SELECT b.parent::regclass,
7171
regexp_replace(b.indname::text, '(pg_toast.pg_toast_)\\d+(_index)', '\\1<oid>\\2'),
72+
CASE WHEN a.oid = b.indoid THEN 'OID is unchanged'
73+
ELSE 'OID has changed' END,
7274
CASE WHEN a.relfilenode = b.relfilenode THEN 'relfilenode is unchanged'
7375
ELSE 'relfilenode has changed' END
7476
FROM index_relfilenodes b
@@ -83,10 +85,10 @@
8385
'SQL REINDEX run');
8486
my $relnode_info = $node->safe_psql('postgres', $compare_relfilenodes);
8587
is( $relnode_info,
86-
qq(pg_constraint|pg_constraint_oid_index|relfilenode is unchanged
87-
pg_constraint|pg_toast.pg_toast_<oid>_index|relfilenode is unchanged
88-
test1|pg_toast.pg_toast_<oid>_index|relfilenode has changed
89-
test1|test1x|relfilenode has changed),
88+
qq(pg_constraint|pg_constraint_oid_index|OID is unchanged|relfilenode is unchanged
89+
pg_constraint|pg_toast.pg_toast_<oid>_index|OID is unchanged|relfilenode is unchanged
90+
test1|pg_toast.pg_toast_<oid>_index|OID is unchanged|relfilenode has changed
91+
test1|test1x|OID is unchanged|relfilenode has changed),
9092
'relfilenode change after REINDEX DATABASE');
9193

9294
# Re-save and run the second one.
@@ -98,10 +100,10 @@
98100
'reindex system tables');
99101
$relnode_info = $node->safe_psql('postgres', $compare_relfilenodes);
100102
is( $relnode_info,
101-
qq(pg_constraint|pg_constraint_oid_index|relfilenode has changed
102-
pg_constraint|pg_toast.pg_toast_<oid>_index|relfilenode has changed
103-
test1|pg_toast.pg_toast_<oid>_index|relfilenode is unchanged
104-
test1|test1x|relfilenode is unchanged),
103+
qq(pg_constraint|pg_constraint_oid_index|OID is unchanged|relfilenode has changed
104+
pg_constraint|pg_toast.pg_toast_<oid>_index|OID is unchanged|relfilenode has changed
105+
test1|pg_toast.pg_toast_<oid>_index|OID is unchanged|relfilenode is unchanged
106+
test1|test1x|OID is unchanged|relfilenode is unchanged),
105107
'relfilenode change after REINDEX SYSTEM');
106108

107109
$node->issues_sql_like(
@@ -132,11 +134,22 @@
132134
qr/statement: REINDEX \(VERBOSE, TABLESPACE $tbspace_name\) TABLE public\.test1;/,
133135
'reindex with verbose output and tablespace');
134136

135-
# the same with --concurrently
137+
# Same with --concurrently.
138+
# Save the state of the relations and compare them after the DATABASE
139+
# rebuild.
140+
$node->safe_psql('postgres',
141+
"TRUNCATE index_relfilenodes; $save_relfilenodes");
136142
$node->issues_sql_like(
137143
[ 'reindexdb', '--concurrently', 'postgres' ],
138144
qr/statement: REINDEX DATABASE CONCURRENTLY postgres;/,
139145
'SQL REINDEX CONCURRENTLY run');
146+
$relnode_info = $node->safe_psql('postgres', $compare_relfilenodes);
147+
is( $relnode_info,
148+
qq(pg_constraint|pg_constraint_oid_index|OID is unchanged|relfilenode is unchanged
149+
pg_constraint|pg_toast.pg_toast_<oid>_index|OID is unchanged|relfilenode is unchanged
150+
test1|pg_toast.pg_toast_<oid>_index|OID has changed|relfilenode has changed
151+
test1|test1x|OID has changed|relfilenode has changed),
152+
'OID change after REINDEX DATABASE CONCURRENTLY');
140153

141154
$node->issues_sql_like(
142155
[ 'reindexdb', '--concurrently', '-t', 'test1', 'postgres' ],

src/test/regress/expected/create_index.out

+4-3
Original file line numberDiff line numberDiff line change
@@ -2521,16 +2521,17 @@ ERROR: cannot reindex system catalogs concurrently
25212521
REINDEX INDEX CONCURRENTLY pg_toast.pg_toast_1260_index; -- no catalog toast index
25222522
ERROR: cannot reindex system catalogs concurrently
25232523
REINDEX SYSTEM CONCURRENTLY postgres; -- not allowed for SYSTEM
2524-
ERROR: syntax error at or near "CONCURRENTLY"
2525-
LINE 1: REINDEX SYSTEM CONCURRENTLY postgres;
2526-
^
2524+
ERROR: cannot reindex system catalogs concurrently
25272525
REINDEX (CONCURRENTLY) SYSTEM postgres; -- ditto
25282526
ERROR: cannot reindex system catalogs concurrently
25292527
REINDEX (CONCURRENTLY) SYSTEM; -- ditto
25302528
ERROR: cannot reindex system catalogs concurrently
25312529
-- Warns about catalog relations
25322530
REINDEX SCHEMA CONCURRENTLY pg_catalog;
25332531
WARNING: cannot reindex system catalogs concurrently, skipping all
2532+
-- Not the current database
2533+
REINDEX DATABASE not_current_database;
2534+
ERROR: can only reindex the currently open database
25342535
-- Check the relation status, there should not be invalid indexes
25352536
\d concur_reindex_tab
25362537
Table "public.concur_reindex_tab"

src/test/regress/sql/create_index.sql

+2
Original file line numberDiff line numberDiff line change
@@ -1076,6 +1076,8 @@ REINDEX (CONCURRENTLY) SYSTEM postgres; -- ditto
10761076
REINDEX (CONCURRENTLY) SYSTEM; -- ditto
10771077
-- Warns about catalog relations
10781078
REINDEX SCHEMA CONCURRENTLY pg_catalog;
1079+
-- Not the current database
1080+
REINDEX DATABASE not_current_database;
10791081

10801082
-- Check the relation status, there should not be invalid indexes
10811083
\d concur_reindex_tab

0 commit comments

Comments
 (0)