Fix assertion failure with REINDEX and event triggers

A REINDEX CONCURRENTLY run on a table with no indexes would always pop
the topmost snapshot from the active snapshot stack, making the snapshot
handling inconsistent between the multiple-relation and single-relation
cases.  This commit slightly changes the snapshot stack handling so as a
snapshot is popped only ReindexMultipleInternal() in this case after a
relation has been reindexed, fixing a problem where an event trigger
function may need a snapshot but does not have one.  This also keeps the
places where PopActiveSnapshot() is called closer to each other.

While on it, this expands the existing tests to cover all the cases that
could be faced with REINDEX commands and such event triggers, for one or
more relations, with or without indexes.

This behavior is inconsistent since 5dc92b8, but we've never had a
need for an active snapshot at the end of a REINDEX until now.

Thanks also to Jian He for the input.

Reported-by: Alexander Lakhin
Discussion: https://postgr.es/m/cb538743-484c-eb6a-a8c5-359980cd3a17@gmail.com

Author: michaelpq

src/backend/commands/indexcmds.c

@@ -3347,6 +3347,8 @@ ReindexMultipleInternal(const ReindexStmt *stmt, const List *relids, const Reind
 
 			newparams.options |= REINDEXOPT_MISSING_OK;
 			(void) ReindexRelationConcurrently(stmt, relid, &newparams);
+			if (ActiveSnapshotSet())
+				PopActiveSnapshot();
 			/* ReindexRelationConcurrently() does the verbose output */
 		}
 		else if (relkind == RELKIND_INDEX)
@@ -3698,10 +3700,7 @@ ReindexRelationConcurrently(const ReindexStmt *stmt, Oid relationOid, const Rein
 	 * session until this operation completes.
 	 */
 	if (indexIds == NIL)
-	{
-		PopActiveSnapshot();
 		return false;
-	}
 
 	/* It's not a shared catalog, so refuse to move it to shared tablespace */
 	if (params->tablespaceOid == GLOBALTABLESPACE_OID)

src/test/regress/expected/event_trigger.out

@@ -581,6 +581,12 @@ $$ LANGUAGE plpgsql;
 CREATE EVENT TRIGGER regress_reindex_end ON ddl_command_end
     WHEN TAG IN ('REINDEX')
     EXECUTE PROCEDURE reindex_end_command();
+-- Extra event to force the use of a snapshot.
+CREATE FUNCTION reindex_end_command_snap() RETURNS EVENT_TRIGGER
+    AS $$ BEGIN PERFORM 1; END $$ LANGUAGE plpgsql;
+CREATE EVENT TRIGGER regress_reindex_end_snap ON ddl_command_end
+    EXECUTE FUNCTION reindex_end_command_snap();
+-- With simple relation
 CREATE TABLE concur_reindex_tab (c1 int);
 CREATE INDEX concur_reindex_ind ON concur_reindex_tab (c1);
 -- Both start and end triggers enabled.
@@ -602,10 +608,56 @@ REINDEX INDEX concur_reindex_ind;
 NOTICE:  REINDEX END: command_tag=REINDEX type=index identity=public.concur_reindex_ind
 REINDEX INDEX CONCURRENTLY concur_reindex_ind;
 NOTICE:  REINDEX END: command_tag=REINDEX type=index identity=public.concur_reindex_ind
+-- without an index
+DROP INDEX concur_reindex_ind;
+REINDEX TABLE concur_reindex_tab;
+NOTICE:  table "concur_reindex_tab" has no indexes to reindex
+REINDEX TABLE CONCURRENTLY concur_reindex_tab;
+NOTICE:  table "concur_reindex_tab" has no indexes that can be reindexed concurrently
+-- With a Schema
+CREATE SCHEMA concur_reindex_schema;
+-- No indexes
+REINDEX SCHEMA concur_reindex_schema;
+REINDEX SCHEMA CONCURRENTLY concur_reindex_schema;
+CREATE TABLE concur_reindex_schema.tab (a int);
+CREATE INDEX ind ON concur_reindex_schema.tab (a);
+-- One index reported
+REINDEX SCHEMA concur_reindex_schema;
+NOTICE:  REINDEX END: command_tag=REINDEX type=index identity=concur_reindex_schema.ind
+REINDEX SCHEMA CONCURRENTLY concur_reindex_schema;
+NOTICE:  REINDEX END: command_tag=REINDEX type=index identity=concur_reindex_schema.ind
+-- One table on schema but no indexes
+DROP INDEX concur_reindex_schema.ind;
+REINDEX SCHEMA concur_reindex_schema;
+REINDEX SCHEMA CONCURRENTLY concur_reindex_schema;
+DROP SCHEMA concur_reindex_schema CASCADE;
+NOTICE:  drop cascades to table concur_reindex_schema.tab
+-- With a partitioned table, and nothing else.
+CREATE TABLE concur_reindex_part (id int) PARTITION BY RANGE (id);
+REINDEX TABLE concur_reindex_part;
+REINDEX TABLE CONCURRENTLY concur_reindex_part;
+-- Partition that would be reindexed, still nothing.
+CREATE TABLE concur_reindex_child PARTITION OF concur_reindex_part
+  FOR VALUES FROM (0) TO (10);
+REINDEX TABLE concur_reindex_part;
+REINDEX TABLE CONCURRENTLY concur_reindex_part;
+-- Now add some indexes.
+CREATE INDEX concur_reindex_partidx ON concur_reindex_part (id);
+REINDEX INDEX concur_reindex_partidx;
+NOTICE:  REINDEX END: command_tag=REINDEX type=index identity=public.concur_reindex_child_id_idx
+REINDEX INDEX CONCURRENTLY concur_reindex_partidx;
+NOTICE:  REINDEX END: command_tag=REINDEX type=index identity=public.concur_reindex_child_id_idx
+REINDEX TABLE concur_reindex_part;
+NOTICE:  REINDEX END: command_tag=REINDEX type=index identity=public.concur_reindex_child_id_idx
+REINDEX TABLE CONCURRENTLY concur_reindex_part;
+NOTICE:  REINDEX END: command_tag=REINDEX type=index identity=public.concur_reindex_child_id_idx
+DROP TABLE concur_reindex_part;
 -- Clean up
 DROP EVENT TRIGGER regress_reindex_start;
 DROP EVENT TRIGGER regress_reindex_end;
+DROP EVENT TRIGGER regress_reindex_end_snap;
 DROP FUNCTION reindex_end_command();
+DROP FUNCTION reindex_end_command_snap();
 DROP FUNCTION reindex_start_command();
 DROP TABLE concur_reindex_tab;
 -- test Row Security Event Trigger

src/test/regress/sql/event_trigger.sql

@@ -443,7 +443,13 @@ $$ LANGUAGE plpgsql;
 CREATE EVENT TRIGGER regress_reindex_end ON ddl_command_end
     WHEN TAG IN ('REINDEX')
     EXECUTE PROCEDURE reindex_end_command();
+-- Extra event to force the use of a snapshot.
+CREATE FUNCTION reindex_end_command_snap() RETURNS EVENT_TRIGGER
+    AS $$ BEGIN PERFORM 1; END $$ LANGUAGE plpgsql;
+CREATE EVENT TRIGGER regress_reindex_end_snap ON ddl_command_end
+    EXECUTE FUNCTION reindex_end_command_snap();
 
+-- With simple relation
 CREATE TABLE concur_reindex_tab (c1 int);
 CREATE INDEX concur_reindex_ind ON concur_reindex_tab (c1);
 -- Both start and end triggers enabled.
@@ -455,11 +461,50 @@ REINDEX TABLE CONCURRENTLY concur_reindex_tab;
 ALTER EVENT TRIGGER regress_reindex_start DISABLE;
 REINDEX INDEX concur_reindex_ind;
 REINDEX INDEX CONCURRENTLY concur_reindex_ind;
+-- without an index
+DROP INDEX concur_reindex_ind;
+REINDEX TABLE concur_reindex_tab;
+REINDEX TABLE CONCURRENTLY concur_reindex_tab;
+
+-- With a Schema
+CREATE SCHEMA concur_reindex_schema;
+-- No indexes
+REINDEX SCHEMA concur_reindex_schema;
+REINDEX SCHEMA CONCURRENTLY concur_reindex_schema;
+CREATE TABLE concur_reindex_schema.tab (a int);
+CREATE INDEX ind ON concur_reindex_schema.tab (a);
+-- One index reported
+REINDEX SCHEMA concur_reindex_schema;
+REINDEX SCHEMA CONCURRENTLY concur_reindex_schema;
+-- One table on schema but no indexes
+DROP INDEX concur_reindex_schema.ind;
+REINDEX SCHEMA concur_reindex_schema;
+REINDEX SCHEMA CONCURRENTLY concur_reindex_schema;
+DROP SCHEMA concur_reindex_schema CASCADE;
+
+-- With a partitioned table, and nothing else.
+CREATE TABLE concur_reindex_part (id int) PARTITION BY RANGE (id);
+REINDEX TABLE concur_reindex_part;
+REINDEX TABLE CONCURRENTLY concur_reindex_part;
+-- Partition that would be reindexed, still nothing.
+CREATE TABLE concur_reindex_child PARTITION OF concur_reindex_part
+  FOR VALUES FROM (0) TO (10);
+REINDEX TABLE concur_reindex_part;
+REINDEX TABLE CONCURRENTLY concur_reindex_part;
+-- Now add some indexes.
+CREATE INDEX concur_reindex_partidx ON concur_reindex_part (id);
+REINDEX INDEX concur_reindex_partidx;
+REINDEX INDEX CONCURRENTLY concur_reindex_partidx;
+REINDEX TABLE concur_reindex_part;
+REINDEX TABLE CONCURRENTLY concur_reindex_part;
+DROP TABLE concur_reindex_part;
 
 -- Clean up
 DROP EVENT TRIGGER regress_reindex_start;
 DROP EVENT TRIGGER regress_reindex_end;
+DROP EVENT TRIGGER regress_reindex_end_snap;
 DROP FUNCTION reindex_end_command();
+DROP FUNCTION reindex_end_command_snap();
 DROP FUNCTION reindex_start_command();
 DROP TABLE concur_reindex_tab;