On Tue, Dec 05, 2023 at 12:49:12AM +0800, jian he wrote:
> On Mon, Dec 4, 2023 at 11:00 PM Alexander Lakhin <exclus...@gmail.com> wrote:
>> Please look at the assertion failure triggered when REINDEX processed by an
>> event trigger:
>> CREATE FUNCTION etf() RETURNS EVENT_TRIGGER AS $$ BEGIN PERFORM 1; END $$
>> LANGUAGE plpgsql;
>> CREATE EVENT TRIGGER et ON ddl_command_end EXECUTE FUNCTION etf();
>> CREATE TABLE t (i int);
>> REINDEX (CONCURRENTLY) TABLE t;
>
> In indexcmd.c, function, ReindexRelationConcurrently
> if (indexIds == NIL)
> {
> PopActiveSnapshot();
> return false;
> }
>
> So there is no snapshot left, then PERFORM 1; need a snapshot.Popping a snapshot at this stage when there are no indexes has been a decision taken by the original commit in 5dc92b844e68 because we had no need for it yet, but we may do now depending on the function triggered. I have been looking at the whole stack and something like the attached to make a pop conditional seems to be sufficient to satisfy all the cases I've been able to come up with, including the one reported here. Alexander, do you see any hole in that? Perhaps the snapshot push should be more aggressive at the end of ReindexRelationConcurrently() as well (in the last transaction when rebuilds happen)? -- Michael
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 412e1ba84f..4ee498d985 100644
--- a/src/backend/commands/indexcmds.c
+++ b/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)
diff --git a/src/test/regress/expected/event_trigger.out b/src/test/regress/expected/event_trigger.out
index fdcd127945..a6ce337be5 100644
--- a/src/test/regress/expected/event_trigger.out
+++ b/src/test/regress/expected/event_trigger.out
@@ -581,6 +581,36 @@ $$ 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 a Schema
+CREATE SCHEMA concur_reindex_schema;
+-- No indexes
+REINDEX SCHEMA concur_reindex_schema;
+NOTICE: REINDEX START: ddl_command_start REINDEX
+REINDEX SCHEMA CONCURRENTLY concur_reindex_schema;
+NOTICE: REINDEX START: ddl_command_start REINDEX
+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 START: ddl_command_start REINDEX
+NOTICE: REINDEX END: command_tag=REINDEX type=index identity=concur_reindex_schema.ind
+REINDEX SCHEMA CONCURRENTLY concur_reindex_schema;
+NOTICE: REINDEX START: ddl_command_start REINDEX
+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;
+NOTICE: REINDEX START: ddl_command_start REINDEX
+REINDEX SCHEMA CONCURRENTLY concur_reindex_schema;
+NOTICE: REINDEX START: ddl_command_start REINDEX
+DROP SCHEMA concur_reindex_schema CASCADE;
+NOTICE: drop cascades to table concur_reindex_schema.tab
+-- With 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 +632,18 @@ 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
-- 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
diff --git a/src/test/regress/sql/event_trigger.sql b/src/test/regress/sql/event_trigger.sql
index e5c8bf8412..18bac8820d 100644
--- a/src/test/regress/sql/event_trigger.sql
+++ b/src/test/regress/sql/event_trigger.sql
@@ -443,7 +443,29 @@ $$ 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 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 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 +477,17 @@ 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;
-- 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;
signature.asc
Description: PGP signature
