agora inbox for [email protected]  

help / color / mirror / Atom feed

[PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
43+ messages / 4 participants
[nested] [flat]

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH 2/2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index b1ce83e1dd..c5f257ce15 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -122,6 +122,7 @@ typedef struct ReindexIndexInfo
 	Oid			indexId;
 	Oid			tableId;
 	Oid			amId;
+	bool		safe;			/* for set_indexsafe_procflags */
 } ReindexIndexInfo;
 
 /*
@@ -395,7 +396,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1574,9 +1575,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only takes a snapshot to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3343,6 +3346,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3384,6 +3390,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3451,6 +3458,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3480,6 +3492,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3500,8 +3516,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3530,6 +3552,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3564,6 +3590,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * To ensure no deadlocks, we must commit and start yet another
 		 * transaction, and do our wait before any snapshot has been taken in
 		 * it.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		CommitTransactionCommand();
 		StartTransactionCommand();
@@ -3594,6 +3623,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3641,6 +3677,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only takes a snapshot to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3671,6 +3713,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only takes a snapshot to do some catalog manipulations, after all
+	 * the waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index e77f76ae8a..e1a6bc5170 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--GvXjxJ+pjyke8COw--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
@ 2020-11-30 19:01 Alvaro Herrera <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2020-11-30 19:01 UTC (permalink / raw)

---
 src/backend/commands/indexcmds.c | 56 +++++++++++++++++++++++++++++---
 src/include/storage/proc.h       |  1 +
 2 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 8c9c39a467..35c3d20eae 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -385,7 +385,7 @@ CompareOpclassOptions(Datum *opts1, Datum *opts2, int natts)
  * lazy VACUUMs, because they won't be fazed by missing index entries
  * either.  (Manual ANALYZEs, however, can't be excluded because they
  * might be within transactions that are going to do arbitrary operations
- * later.)  Processes running CREATE INDEX CONCURRENTLY
+ * later.)  Processes running CREATE INDEX CONCURRENTLY or REINDEX CONCURRENTLY
  * on indexes that are neither expressional nor partial are also safe to
  * ignore, since we know that those processes won't examine any data
  * outside the table they're indexing.
@@ -1566,9 +1566,11 @@ DefineIndex(Oid relationId,
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
-	/* Tell concurrent index builds to ignore us, if index qualifies */
-	if (safe_index)
-		set_indexsafe_procflags();
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
 
 	/* We should now definitely not be advertising any xmin. */
 	Assert(MyProc->xmin == InvalidTransactionId);
@@ -3066,6 +3068,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		Oid			indexId;
 		Oid			tableId;
 		Oid			amId;
+		bool		safe;		/* for set_indexsafe_procflags */
 	} ReindexIndexInfo;
 	List	   *heapRelationIds = NIL;
 	List	   *indexIds = NIL;
@@ -3377,6 +3380,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		heapRel = table_open(indexRel->rd_index->indrelid,
 							 ShareUpdateExclusiveLock);
 
+		/* determine safety of this index for set_indexsafe_procflags */
+		idx->safe = (indexRel->rd_indexprs == NIL &&
+					 indexRel->rd_indpred == NIL);
 		idx->tableId = RelationGetRelid(heapRel);
 		idx->amId = indexRel->rd_rel->relam;
 
@@ -3418,6 +3424,7 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 		newidx = palloc(sizeof(ReindexIndexInfo));
 		newidx->indexId = newIndexId;
+		newidx->safe = idx->safe;
 		newidx->tableId = idx->tableId;
 		newidx->amId = idx->amId;
 
@@ -3485,6 +3492,11 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 2 of REINDEX CONCURRENTLY
 	 *
@@ -3514,6 +3526,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/* Set ActiveSnapshot since functions in the indexes may need it */
 		PushActiveSnapshot(GetTransactionSnapshot());
 
@@ -3534,8 +3550,14 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		PopActiveSnapshot();
 		CommitTransactionCommand();
 	}
+
 	StartTransactionCommand();
 
+	/*
+	 * Because we don't take a snapshot in this transaction, there's no need
+	 * to set the PROC_IN_SAFE_IC flag here.
+	 */
+
 	/*
 	 * Phase 3 of REINDEX CONCURRENTLY
 	 *
@@ -3564,6 +3586,10 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 */
 		CHECK_FOR_INTERRUPTS();
 
+		/* Tell concurrent indexing to ignore us, if index qualifies */
+		if (newidx->safe)
+			set_indexsafe_procflags();
+
 		/*
 		 * Take the "reference snapshot" that will be used by validate_index()
 		 * to filter candidate tuples.
@@ -3607,6 +3633,9 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 		 * interesting tuples.  But since it might not contain tuples deleted
 		 * just before the reference snap was taken, we have to wait out any
 		 * transactions that might have older snapshots.
+		 *
+		 * Because we don't take a snapshot in this transaction, there's no
+		 * need to set the PROC_IN_SAFE_IC flag here.
 		 */
 		pgstat_progress_update_param(PROGRESS_CREATEIDX_PHASE,
 									 PROGRESS_CREATEIDX_PHASE_WAIT_3);
@@ -3628,6 +3657,13 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 
 	StartTransactionCommand();
 
+	/*
+	 * Because this transaction only does catalog manipulations and doesn't do
+	 * any index operations, we can set the PROC_IN_SAFE_IC flag here
+	 * unconditionally.
+	 */
+	set_indexsafe_procflags();
+
 	forboth(lc, indexIds, lc2, newIndexIds)
 	{
 		ReindexIndexInfo *oldidx = lfirst(lc);
@@ -3675,6 +3711,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after the
+	 * wait is over.
+	 */
+
 	/*
 	 * Phase 5 of REINDEX CONCURRENTLY
 	 *
@@ -3705,6 +3747,12 @@ ReindexRelationConcurrently(Oid relationOid, int options)
 	CommitTransactionCommand();
 	StartTransactionCommand();
 
+	/*
+	 * This transaction doesn't need to set the PROC_IN_SAFE_IC flag, because
+	 * it only acquires an Xid to do some catalog manipulations, after all the
+	 * waiting has been completed.
+	 */
+
 	/*
 	 * Phase 6 of REINDEX CONCURRENTLY
 	 *
diff --git a/src/include/storage/proc.h b/src/include/storage/proc.h
index 0786fcf103..683ab64f76 100644
--- a/src/include/storage/proc.h
+++ b/src/include/storage/proc.h
@@ -54,6 +54,7 @@ struct XidCache
 #define		PROC_IS_AUTOVACUUM	0x01	/* is it an autovac worker? */
 #define		PROC_IN_VACUUM		0x02	/* currently running lazy vacuum */
 #define		PROC_IN_SAFE_IC		0x04	/* currently running CREATE INDEX
+										 * CONCURRENTLY or REINDEX
 										 * CONCURRENTLY on non-expressional,
 										 * non-partial index */
 #define		PROC_VACUUM_FOR_WRAPAROUND	0x08	/* set by autovac only */
-- 
2.20.1


--tKW2IUtsqtDRztdT--





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
@ 2024-07-02 09:05 Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  0 siblings, 1 reply; 43+ messages in thread

From: Bertrand Drouvot @ 2024-07-02 09:05 UTC (permalink / raw)
  To: Sami Imseih <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

Hi,

On Fri, Jun 28, 2024 at 05:50:20PM -0500, Sami Imseih wrote:
> 
> Thanks for the feedback!
> 
> > On Jun 28, 2024, at 4:34 PM, Tom Lane <[email protected]> wrote:
> > 
> > Sami Imseih <[email protected]> writes:
> >> Reattaching the patch.
> > 
> > I feel like this is fundamentally a wrong solution, for the reasons
> > cited in the comment for pg_usleep: long sleeps are a bad idea
> > because of the resulting uncertainty about whether we'll respond to
> > interrupts and such promptly.  An example here is that if we get
> > a query cancel interrupt, we should probably not insist on finishing
> > out the current sleep before responding.
> 
> The case which brought up this discussion is the pg_usleep that 
> is called within the vacuum_delay_point being interrupted. 
> 
> When I read the same code comment you cited, it sounded to me 
> that “long sleeps” are those that are in seconds or minutes. The longest 
> vacuum delay allowed is 100ms.

I think that with the proposed patch the actual wait time can be "long".

Indeed, the time between the interruptions and restarts of the nanosleep() call
will lead to drift (as mentioned in the nanosleep() man page). So, with a large
number of interruptions, the actual wait time could be way longer than the
expected wait time.

To put numbers, I did a few tests with the patch (and with v2 shared in [1]):

cost delay is 1ms and cost limit is 10.

With 50 indexes and 10 parallel workers I can see things like:

2024-07-02 08:22:23.789 UTC [2189616] LOG:  expected 1.000000, actual 239.378368
2024-07-02 08:22:24.575 UTC [2189616] LOG:  expected 0.100000, actual 224.331737
2024-07-02 08:22:25.363 UTC [2189616] LOG:  expected 1.300000, actual 230.462793
2024-07-02 08:22:26.154 UTC [2189616] LOG:  expected 1.000000, actual 225.980803

Means we waited more than the max allowed cost delay (100ms).

With 49 parallel workers, it's worst as I can see things like:

2024-07-02 08:26:36.069 UTC [2189807] LOG:  expected 1.000000, actual 1106.790136
2024-07-02 08:26:36.298 UTC [2189807] LOG:  expected 1.000000, actual 218.148985

The first actual wait time is about 1 second (it has been interrupted about
16300 times during this second).

To avoid this drift, the nanosleep() man page suggests to use clock_nanosleep()
with an absolute time value, that might be another idea to explore.

[1]: https://www.postgresql.org/message-id/flat/ZmaXmWDL829fzAVX%40ip-10-97-1-34.eu-west-3.compute.intern...

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
@ 2024-07-05 16:49 ` Sami Imseih <[email protected]>
  2024-07-05 16:57   ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  0 siblings, 2 replies; 43+ messages in thread

From: Sami Imseih @ 2024-07-05 16:49 UTC (permalink / raw)
  To: Bertrand Drouvot <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers


> With 50 indexes and 10 parallel workers I can see things like:
> 
> 2024-07-02 08:22:23.789 UTC [2189616] LOG:  expected 1.000000, actual 239.378368
> 2024-07-02 08:22:24.575 UTC [2189616] LOG:  expected 0.100000, actual 224.331737
> 2024-07-02 08:22:25.363 UTC [2189616] LOG:  expected 1.300000, actual 230.462793
> 2024-07-02 08:22:26.154 UTC [2189616] LOG:  expected 1.000000, actual 225.980803
> 
> Means we waited more than the max allowed cost delay (100ms).
> 
> With 49 parallel workers, it's worst as I can see things like:
> 
> 2024-07-02 08:26:36.069 UTC [2189807] LOG:  expected 1.000000, actual 1106.790136
> 2024-07-02 08:26:36.298 UTC [2189807] LOG:  expected 1.000000, actual 218.148985
> 
> The first actual wait time is about 1 second (it has been interrupted about
> 16300 times during this second).
> 
> To avoid this drift, the nanosleep() man page suggests to use clock_nanosleep()
> with an absolute time value, that might be another idea to explore.
> 
> [1]: https://www.postgresql.org/message-id/flat/ZmaXmWDL829fzAVX%40ip-10-97-1-34.eu-west-3.compute.intern...
> 



I could not reproduce the same time you drift you observed on my
machine, so I am guessing the time drift could be worse on certain
platforms than others.

I also looked into the WaitLatchUs patch proposed by Thomas in [1]
and since my system does have epoll_pwait(2) available, I could not
achieve the sub-millisecond wait times. 

A more portable approach which could be to continue using nanosleep and
add checks to ensure that nanosleep exists whenever
it goes past an absolute time. This was suggested by Bertrand in an offline
conversation. I am not yet fully convinced of this idea, but posting the patch
that implements this idea for anyone interested in looking.

Since sub-millisecond sleep times are not guaranteed as suggested by
the vacuum_cost_delay docs ( see below ), an alternative idea
is to use clock_nanosleep for vacuum delay when it’s available, else
fallback to WaitLatch.

"While vacuum_cost_delay can be set to fractional-millisecond values, 
such delays may not be measured accurately on older platforms”



[1] https://www.postgresql.org/message-id/CA%2BhUKGKVbJE59JkwnUj5XMY%2B-rzcTFciV9vVC7i%3DLUfWPds8Xw%40ma...

Regards, 

Sami



Attachments:

  [application/octet-stream] 0001-vaccum_delay-with-absolute-time-nanosleep.patch (3.5K, ../../[email protected]/3-0001-vaccum_delay-with-absolute-time-nanosleep.patch)
  download | inline diff:
From 942de12a49f53e20ab460d68191a6dc576f0750b Mon Sep 17 00:00:00 2001
From: EC2 Default User <[email protected]>
Date: Fri, 5 Jul 2024 16:47:21 +0000
Subject: [PATCH 1/1] vaccum_delay with absolute time nanosleep

---
 src/backend/commands/vacuum.c | 74 +++++++++++++++++++++++++++++------
 1 file changed, 63 insertions(+), 11 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 48f8eab202..bfa024f583 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -40,6 +40,7 @@
 #include "catalog/pg_inherits.h"
 #include "commands/cluster.h"
 #include "commands/defrem.h"
+#include "commands/progress.h"
 #include "commands/vacuum.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
@@ -116,6 +117,58 @@ static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 static double compute_parallel_delay(void);
 static VacOptValue get_vacoptval_from_boolean(DefElem *def);
 static bool vac_tid_reaped(ItemPointer itemptr, void *state);
+static void vacuum_sleep(double msec);
+
+static
+void vacuum_sleep(double msec)
+{
+	long microsec = msec * 1000;
+
+	if (microsec > 0)
+	{
+#ifndef WIN32
+		/*
+		 * We allow nanosleep to handle interrupts and retry with the remaining time.
+		 * However, since nanosleep is susceptible to time drift when interrupted
+		 * frequently, we add a safeguard to break out of the nanosleep whenever the
+		 * current time is past the absolute time. The absolute time for sleeping is
+		 * set before the nanosleep loop starts and the current time is checked
+		 * within the loop whenever nanosleep encounters an interrupt.
+		 */
+		struct timespec delay;
+		struct timespec remain;
+		struct timespec absolute;
+
+		clock_gettime(PG_INSTR_CLOCK, &absolute);
+
+		absolute.tv_sec += microsec / 1000000L;
+		absolute.tv_nsec += (microsec % 1000000L) * 1000;
+
+		delay.tv_sec = microsec / 1000000L;
+		delay.tv_nsec = (microsec % 1000000L) * 1000;
+
+		while(nanosleep(&delay, &remain) == -1 && errno == EINTR)
+		{
+			struct timespec current;
+			float time_diff;
+
+			clock_gettime(PG_INSTR_CLOCK, &current);
+
+			time_diff = (absolute.tv_sec - current.tv_sec) + (absolute.tv_nsec - current.tv_nsec) / 1000000000.0;
+
+			if (time_diff <= 0)
+				break;
+
+			delay = remain;
+		}
+#else
+		SleepEx((microsec < 500 ? 1 : (microsec + 500) / 1000), FALSE);
+#endif
+	}
+
+	if (IsUnderPostmaster && !PostmasterIsAlive())
+		exit(1);
+}
 
 /*
  * GUC check function to ensure GUC value specified is within the allowable
@@ -2380,21 +2433,20 @@ vacuum_delay_point(void)
 	/* Nap if appropriate */
 	if (msec > 0)
 	{
+		instr_time	delay_start;
+		instr_time	delay_end;
+		instr_time	delayed_time;
+
 		if (msec > vacuum_cost_delay * 4)
 			msec = vacuum_cost_delay * 4;
 
-		pgstat_report_wait_start(WAIT_EVENT_VACUUM_DELAY);
-		pg_usleep(msec * 1000);
-		pgstat_report_wait_end();
+		INSTR_TIME_SET_CURRENT(delay_start);
+		vacuum_sleep(msec);
+		INSTR_TIME_SET_CURRENT(delay_end);
 
-		/*
-		 * We don't want to ignore postmaster death during very long vacuums
-		 * with vacuum_cost_delay configured.  We can't use the usual
-		 * WaitLatch() approach here because we want microsecond-based sleep
-		 * durations above.
-		 */
-		if (IsUnderPostmaster && !PostmasterIsAlive())
-			exit(1);
+		INSTR_TIME_SET_ZERO(delayed_time);
+		INSTR_TIME_ACCUM_DIFF(delayed_time, delay_end, delay_start);
+		elog(LOG, "msec = %lf, delayed_time = %lf", msec, INSTR_TIME_GET_MILLISEC(delayed_time));
 
 		VacuumCostBalance = 0;
 
-- 
2.40.1



^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
@ 2024-07-05 16:57   ` Sami Imseih <[email protected]>
  1 sibling, 0 replies; 43+ messages in thread

From: Sami Imseih @ 2024-07-05 16:57 UTC (permalink / raw)
  To: Bertrand Drouvot <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

> 
> A more portable approach which could be to continue using nanosleep and
> add checks to ensure that nanosleep exists whenever
> it goes past an absolute time. This was suggested by Bertrand in an offline
> conversation. I am not yet fully convinced of this idea, but posting the patch
> that implements this idea for anyone interested in looking.

oops, forgot to attach the patch. Here it is.

Attachments:

  [application/octet-stream] 0001-vaccum_delay-with-absolute-time-nanosleep.patch (3.5K, ../../[email protected]/2-0001-vaccum_delay-with-absolute-time-nanosleep.patch)
  download | inline diff:
From 942de12a49f53e20ab460d68191a6dc576f0750b Mon Sep 17 00:00:00 2001
From: EC2 Default User <[email protected]>
Date: Fri, 5 Jul 2024 16:47:21 +0000
Subject: [PATCH 1/1] vaccum_delay with absolute time nanosleep

---
 src/backend/commands/vacuum.c | 74 +++++++++++++++++++++++++++++------
 1 file changed, 63 insertions(+), 11 deletions(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 48f8eab202..bfa024f583 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -40,6 +40,7 @@
 #include "catalog/pg_inherits.h"
 #include "commands/cluster.h"
 #include "commands/defrem.h"
+#include "commands/progress.h"
 #include "commands/vacuum.h"
 #include "miscadmin.h"
 #include "nodes/makefuncs.h"
@@ -116,6 +117,58 @@ static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 static double compute_parallel_delay(void);
 static VacOptValue get_vacoptval_from_boolean(DefElem *def);
 static bool vac_tid_reaped(ItemPointer itemptr, void *state);
+static void vacuum_sleep(double msec);
+
+static
+void vacuum_sleep(double msec)
+{
+	long microsec = msec * 1000;
+
+	if (microsec > 0)
+	{
+#ifndef WIN32
+		/*
+		 * We allow nanosleep to handle interrupts and retry with the remaining time.
+		 * However, since nanosleep is susceptible to time drift when interrupted
+		 * frequently, we add a safeguard to break out of the nanosleep whenever the
+		 * current time is past the absolute time. The absolute time for sleeping is
+		 * set before the nanosleep loop starts and the current time is checked
+		 * within the loop whenever nanosleep encounters an interrupt.
+		 */
+		struct timespec delay;
+		struct timespec remain;
+		struct timespec absolute;
+
+		clock_gettime(PG_INSTR_CLOCK, &absolute);
+
+		absolute.tv_sec += microsec / 1000000L;
+		absolute.tv_nsec += (microsec % 1000000L) * 1000;
+
+		delay.tv_sec = microsec / 1000000L;
+		delay.tv_nsec = (microsec % 1000000L) * 1000;
+
+		while(nanosleep(&delay, &remain) == -1 && errno == EINTR)
+		{
+			struct timespec current;
+			float time_diff;
+
+			clock_gettime(PG_INSTR_CLOCK, &current);
+
+			time_diff = (absolute.tv_sec - current.tv_sec) + (absolute.tv_nsec - current.tv_nsec) / 1000000000.0;
+
+			if (time_diff <= 0)
+				break;
+
+			delay = remain;
+		}
+#else
+		SleepEx((microsec < 500 ? 1 : (microsec + 500) / 1000), FALSE);
+#endif
+	}
+
+	if (IsUnderPostmaster && !PostmasterIsAlive())
+		exit(1);
+}
 
 /*
  * GUC check function to ensure GUC value specified is within the allowable
@@ -2380,21 +2433,20 @@ vacuum_delay_point(void)
 	/* Nap if appropriate */
 	if (msec > 0)
 	{
+		instr_time	delay_start;
+		instr_time	delay_end;
+		instr_time	delayed_time;
+
 		if (msec > vacuum_cost_delay * 4)
 			msec = vacuum_cost_delay * 4;
 
-		pgstat_report_wait_start(WAIT_EVENT_VACUUM_DELAY);
-		pg_usleep(msec * 1000);
-		pgstat_report_wait_end();
+		INSTR_TIME_SET_CURRENT(delay_start);
+		vacuum_sleep(msec);
+		INSTR_TIME_SET_CURRENT(delay_end);
 
-		/*
-		 * We don't want to ignore postmaster death during very long vacuums
-		 * with vacuum_cost_delay configured.  We can't use the usual
-		 * WaitLatch() approach here because we want microsecond-based sleep
-		 * durations above.
-		 */
-		if (IsUnderPostmaster && !PostmasterIsAlive())
-			exit(1);
+		INSTR_TIME_SET_ZERO(delayed_time);
+		INSTR_TIME_ACCUM_DIFF(delayed_time, delay_end, delay_start);
+		elog(LOG, "msec = %lf, delayed_time = %lf", msec, INSTR_TIME_GET_MILLISEC(delayed_time));
 
 		VacuumCostBalance = 0;
 
-- 
2.40.1



^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
@ 2024-07-09 10:44   ` Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  1 sibling, 1 reply; 43+ messages in thread

From: Bertrand Drouvot @ 2024-07-09 10:44 UTC (permalink / raw)
  To: Sami Imseih <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

Hi,

On Fri, Jul 05, 2024 at 11:49:45AM -0500, Sami Imseih wrote:
> 
> > With 50 indexes and 10 parallel workers I can see things like:
> > 
> > 2024-07-02 08:22:23.789 UTC [2189616] LOG:  expected 1.000000, actual 239.378368
> > 2024-07-02 08:22:24.575 UTC [2189616] LOG:  expected 0.100000, actual 224.331737
> > 2024-07-02 08:22:25.363 UTC [2189616] LOG:  expected 1.300000, actual 230.462793
> > 2024-07-02 08:22:26.154 UTC [2189616] LOG:  expected 1.000000, actual 225.980803
> > 
> > Means we waited more than the max allowed cost delay (100ms).
> > 
> > With 49 parallel workers, it's worst as I can see things like:
> > 
> > 2024-07-02 08:26:36.069 UTC [2189807] LOG:  expected 1.000000, actual 1106.790136
> > 2024-07-02 08:26:36.298 UTC [2189807] LOG:  expected 1.000000, actual 218.148985
> > 
> > The first actual wait time is about 1 second (it has been interrupted about
> > 16300 times during this second).
> > 
> > To avoid this drift, the nanosleep() man page suggests to use clock_nanosleep()
> > with an absolute time value, that might be another idea to explore.
> > 
> > [1]: https://www.postgresql.org/message-id/flat/ZmaXmWDL829fzAVX%40ip-10-97-1-34.eu-west-3.compute.intern...
> > 
> 
> 
> A more portable approach which could be to continue using nanosleep and
> add checks to ensure that nanosleep exists whenever
> it goes past an absolute time. This was suggested by Bertrand in an offline
> conversation. I am not yet fully convinced of this idea, but posting the patch
> that implements this idea for anyone interested in looking.

Thanks! 

I did a few tests with the patch and did not see any "large" drifts like the
ones observed above.

As far the patch, not thoroughly review (as it's still one option among others
being discussed)):

+           struct timespec current;
+           float time_diff;
+
+           clock_gettime(PG_INSTR_CLOCK, &current);
+
+           time_diff = (absolute.tv_sec - current.tv_sec) + (absolute.tv_nsec - current.tv_nsec) / 1000000000.0;

I think it could be "simplified" by making use of instr_time instead of timespec
for current and absolute. Then I think it would be enough to compare their
ticks.

> Since sub-millisecond sleep times are not guaranteed as suggested by
> the vacuum_cost_delay docs ( see below ), an alternative idea
> is to use clock_nanosleep for vacuum delay when it’s available, else
> fallback to WaitLatch.

Wouldn't that increase even more the cases where sub-millisecond won't be
guaranteed?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
@ 2024-07-11 15:15     ` Sami Imseih <[email protected]>
  2024-07-11 15:19       ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-12 07:54       ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  0 siblings, 2 replies; 43+ messages in thread

From: Sami Imseih @ 2024-07-11 15:15 UTC (permalink / raw)
  To: Bertrand Drouvot <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers


> I did a few tests with the patch and did not see any "large" drifts like the
> ones observed above.

Thanks for testing.

> I think it could be "simplified" by making use of instr_time instead of timespec
> for current and absolute. Then I think it would be enough to compare their
> ticks.

Correct I attached a v2 of this patch that uses instr_time to check the elapsed
time and break out of the loop. It needs some more benchmarking.

>> Since sub-millisecond sleep times are not guaranteed as suggested by
>> the vacuum_cost_delay docs ( see below ), an alternative idea
>> is to use clock_nanosleep for vacuum delay when it’s available, else
>> fallback to WaitLatch.
> 
> Wouldn't that increase even more the cases where sub-millisecond won't be
> guaranteed?

Yes, nanosleep is going to provide the most coverage as it’s widely available.

Regards,


Sami



Attachments:

  [application/octet-stream] v2-0001-vaccum_delay-with-absolute-time-nanosleep.patch (2.3K, ../../[email protected]/3-v2-0001-vaccum_delay-with-absolute-time-nanosleep.patch)
  download | inline diff:
From f0b21250875726bd429bb9657b4037be41f745e4 Mon Sep 17 00:00:00 2001
From: Ubuntu <[email protected]>
Date: Thu, 11 Jul 2024 15:05:37 +0000
Subject: [PATCH 1/1] vaccum_delay with absolute time nanosleep

---
 src/backend/commands/vacuum.c | 49 ++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 48f8eab202..a65295b2ff 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -116,6 +116,53 @@ static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 static double compute_parallel_delay(void);
 static VacOptValue get_vacoptval_from_boolean(DefElem *def);
 static bool vac_tid_reaped(ItemPointer itemptr, void *state);
+static void vacuum_sleep(double msec);
+
+static
+void vacuum_sleep(double msec)
+{
+	long microsec = msec * 1000;
+
+	if (microsec > 0)
+	{
+#ifndef WIN32
+		/*
+		 * We allow nanosleep to handle interrupts and retry with the remaining time.
+		 * However, since nanosleep is susceptible to time drift when interrupted
+		 * frequently, we add a safeguard to break out of the nanosleep whenever the
+		 * total time of the sleep exceeds the requested sleep time. Using nanosleep
+		 * is a more portable approach than clock_nanosleep.
+		 */
+		struct timespec delay;
+		struct timespec remain;
+		struct instr_time start_time;
+
+		INSTR_TIME_SET_CURRENT(start_time);
+
+		delay.tv_sec = microsec / 1000000L;
+		delay.tv_nsec = (microsec % 1000000L) * 1000;
+
+		while(nanosleep(&delay, &remain) == -1 && errno == EINTR)
+		{
+			struct instr_time current_time;
+			struct instr_time elapsed_time;
+
+			INSTR_TIME_SET_CURRENT(current_time);
+
+			INSTR_TIME_SET_ZERO(elapsed_time);
+			INSTR_TIME_ACCUM_DIFF(elapsed_time, current_time, start_time);
+
+			if (INSTR_TIME_GET_MICROSEC(elapsed_time) >= microsec)
+				break;
+
+			delay = remain;
+		}
+
+#else
+		SleepEx((microsec < 500 ? 1 : (microsec + 500) / 1000), FALSE);
+#endif
+	}
+}
 
 /*
  * GUC check function to ensure GUC value specified is within the allowable
@@ -2384,7 +2431,7 @@ vacuum_delay_point(void)
 			msec = vacuum_cost_delay * 4;
 
 		pgstat_report_wait_start(WAIT_EVENT_VACUUM_DELAY);
-		pg_usleep(msec * 1000);
+		vacuum_sleep(msec);
 		pgstat_report_wait_end();
 
 		/*
-- 
2.43.0



^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
@ 2024-07-11 15:19       ` Sami Imseih <[email protected]>
  2024-07-11 15:34         ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  1 sibling, 1 reply; 43+ messages in thread

From: Sami Imseih @ 2024-07-11 15:19 UTC (permalink / raw)
  To: Bertrand Drouvot <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers


> 
> Correct I attached a v2 of this patch that uses instr_time to check the elapsed
> time and break out of the loop. It needs some more benchmarking.

My email client has an issue sending attachments it seems. Reattaching 


Regards,

Sami

Attachments:

  [application/octet-stream] v2-0001-vaccum_delay-with-absolute-time-nanosleep.patch (2.3K, ../../[email protected]/2-v2-0001-vaccum_delay-with-absolute-time-nanosleep.patch)
  download | inline diff:
From f0b21250875726bd429bb9657b4037be41f745e4 Mon Sep 17 00:00:00 2001
From: Ubuntu <[email protected]>
Date: Thu, 11 Jul 2024 15:05:37 +0000
Subject: [PATCH 1/1] vaccum_delay with absolute time nanosleep

---
 src/backend/commands/vacuum.c | 49 ++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 48f8eab202..a65295b2ff 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -116,6 +116,53 @@ static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 static double compute_parallel_delay(void);
 static VacOptValue get_vacoptval_from_boolean(DefElem *def);
 static bool vac_tid_reaped(ItemPointer itemptr, void *state);
+static void vacuum_sleep(double msec);
+
+static
+void vacuum_sleep(double msec)
+{
+	long microsec = msec * 1000;
+
+	if (microsec > 0)
+	{
+#ifndef WIN32
+		/*
+		 * We allow nanosleep to handle interrupts and retry with the remaining time.
+		 * However, since nanosleep is susceptible to time drift when interrupted
+		 * frequently, we add a safeguard to break out of the nanosleep whenever the
+		 * total time of the sleep exceeds the requested sleep time. Using nanosleep
+		 * is a more portable approach than clock_nanosleep.
+		 */
+		struct timespec delay;
+		struct timespec remain;
+		struct instr_time start_time;
+
+		INSTR_TIME_SET_CURRENT(start_time);
+
+		delay.tv_sec = microsec / 1000000L;
+		delay.tv_nsec = (microsec % 1000000L) * 1000;
+
+		while(nanosleep(&delay, &remain) == -1 && errno == EINTR)
+		{
+			struct instr_time current_time;
+			struct instr_time elapsed_time;
+
+			INSTR_TIME_SET_CURRENT(current_time);
+
+			INSTR_TIME_SET_ZERO(elapsed_time);
+			INSTR_TIME_ACCUM_DIFF(elapsed_time, current_time, start_time);
+
+			if (INSTR_TIME_GET_MICROSEC(elapsed_time) >= microsec)
+				break;
+
+			delay = remain;
+		}
+
+#else
+		SleepEx((microsec < 500 ? 1 : (microsec + 500) / 1000), FALSE);
+#endif
+	}
+}
 
 /*
  * GUC check function to ensure GUC value specified is within the allowable
@@ -2384,7 +2431,7 @@ vacuum_delay_point(void)
 			msec = vacuum_cost_delay * 4;
 
 		pgstat_report_wait_start(WAIT_EVENT_VACUUM_DELAY);
-		pg_usleep(msec * 1000);
+		vacuum_sleep(msec);
 		pgstat_report_wait_end();
 
 		/*
-- 
2.43.0



^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:19       ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
@ 2024-07-11 15:34         ` Nathan Bossart <[email protected]>
  2024-07-11 18:10           ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  0 siblings, 1 reply; 43+ messages in thread

From: Nathan Bossart @ 2024-07-11 15:34 UTC (permalink / raw)
  To: Sami Imseih <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

+		/*
+		 * We allow nanosleep to handle interrupts and retry with the remaining time.
+		 * However, since nanosleep is susceptible to time drift when interrupted
+		 * frequently, we add a safeguard to break out of the nanosleep whenever the
+		 * total time of the sleep exceeds the requested sleep time. Using nanosleep
+		 * is a more portable approach than clock_nanosleep.
+		 */

I'm curious why we wouldn't just subtract "elapsed_time" from "delay" at
the bottom of the while loop to avoid needing this extra check.  Also, I
think we need some commentary about why we want to retry after an interrupt
in this case.

-- 
nathan





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:19       ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:34         ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
@ 2024-07-11 18:10           ` Sami Imseih <[email protected]>
  2024-07-11 19:56             ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  0 siblings, 1 reply; 43+ messages in thread

From: Sami Imseih @ 2024-07-11 18:10 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers


> 
> I'm curious why we wouldn't just subtract "elapsed_time" from "delay" at
> the bottom of the while loop to avoid needing this extra check.  

Can you elaborate further? I am not sure how this will work since delay is a timespec 
and elapsed time is an instr_time. 

Also, in every iteration of the loop, the delay must be set to the remaining time. The
purpose of the elapsed_time is to make sure that we don’t surpass requested time
delay as an additional safeguard.

> Also, I
> think we need some commentary about why we want to retry after an interrupt
> in this case.

I will elaborate further in the comments for the next revision.


Regards,

Sami 






^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:19       ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:34         ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-11 18:10           ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
@ 2024-07-11 19:56             ` Nathan Bossart <[email protected]>
  2024-07-12 09:39               ` Re: Restart pg_usleep when interrupted Alvaro Herrera <[email protected]>
  2024-07-12 17:14               ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  0 siblings, 2 replies; 43+ messages in thread

From: Nathan Bossart @ 2024-07-11 19:56 UTC (permalink / raw)
  To: Sami Imseih <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

On Thu, Jul 11, 2024 at 01:10:25PM -0500, Sami Imseih wrote:
>> I'm curious why we wouldn't just subtract "elapsed_time" from "delay" at
>> the bottom of the while loop to avoid needing this extra check.  
> 
> Can you elaborate further? I am not sure how this will work since delay is a timespec 
> and elapsed time is an instr_time. 
> 
> Also, in every iteration of the loop, the delay must be set to the remaining time. The
> purpose of the elapsed_time is to make sure that we don´t surpass requested time
> delay as an additional safeguard.

I'm imagining something like this:

    struct timespec delay;
    TimestampTz end_time;

    end_time = TimestampTzPlusMilliseconds(GetCurrentTimestamp(), msec);

    do
    {
        long        secs;
        int         microsecs;

        TimestampDifference(GetCurrentTimestamp(), end_time,
                            &secs, &microsecs);

        delay.tv_sec = secs;
        delay.tv_nsec = microsecs * 1000;

    } while (nanosleep(&delay, NULL) == -1 && errno == EINTR);

-- 
nathan





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:19       ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:34         ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-11 18:10           ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 19:56             ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
@ 2024-07-12 09:39               ` Alvaro Herrera <[email protected]>
  1 sibling, 0 replies; 43+ messages in thread

From: Alvaro Herrera @ 2024-07-12 09:39 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: Sami Imseih <[email protected]>; Bertrand Drouvot <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

On 2024-Jul-11, Nathan Bossart wrote:

> I'm imagining something like this:
> 
>     struct timespec delay;
>     TimestampTz end_time;
> 
>     end_time = TimestampTzPlusMilliseconds(GetCurrentTimestamp(), msec);
> 
>     do
>     {
>         long        secs;
>         int         microsecs;
> 
>         TimestampDifference(GetCurrentTimestamp(), end_time,
>                             &secs, &microsecs);
> 
>         delay.tv_sec = secs;
>         delay.tv_nsec = microsecs * 1000;
> 
>     } while (nanosleep(&delay, NULL) == -1 && errno == EINTR);

This looks nicer.  We could deal with clock drift easily (in case the
sysadmin winds the clock back) by testing that tv_sec+tv_nsec is not
higher than the initial time to sleep.  I don't know how common this
situation is nowadays, but I remember debugging a system years ago where
autovacuum was sleeping for a very long time because of that.  I can't
remember now if we did anything in the code to cope, or just told
sysadmins not to do that anymore :-)

FWIW my (Linux's) nanosleep() manpage contains this note:

       If  the interval specified in req is not an exact multiple of the granu‐
       larity underlying clock (see time(7)), then the interval will be rounded
       up  to the next multiple.  Furthermore, after the sleep completes, there
       may still be a delay before the CPU becomes free to once  again  execute
       the calling thread.

It's not clear to me what happens if the time to sleep is zero, so maybe
there should be a "if tv_sec == 0 && tv_nsec == 0 then break" statement
at the bottom of the loop, to quit without sleeping one more time than
needed.

For Windows, this [1] looks like an interesting and possibly relevant
read (though maybe SleepEx already does what we want to do here.)

[1] https://randomascii.wordpress.com/2020/10/04/windows-timer-resolution-the-great-rule-change/

-- 
Álvaro Herrera               48°01'N 7°57'E  —  https://www.EnterpriseDB.com/
"Having your biases confirmed independently is how scientific progress is
made, and hence made our great society what it is today" (Mary Gardiner)





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:19       ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:34         ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-11 18:10           ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 19:56             ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
@ 2024-07-12 17:14               ` Sami Imseih <[email protected]>
  2024-07-12 19:18                 ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  1 sibling, 1 reply; 43+ messages in thread

From: Sami Imseih @ 2024-07-12 17:14 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

> 
> I'm imagining something like this:
> 
>    struct timespec delay;
>    TimestampTz end_time;
> 
>    end_time = TimestampTzPlusMilliseconds(GetCurrentTimestamp(), msec);
> 
>    do
>    {
>        long        secs;
>        int         microsecs;
> 
>        TimestampDifference(GetCurrentTimestamp(), end_time,
>                            &secs, &microsecs);
> 
>        delay.tv_sec = secs;
>        delay.tv_nsec = microsecs * 1000;
> 
>    } while (nanosleep(&delay, NULL) == -1 && errno == EINTR);
> 

I do agree that this is cleaner code, but I am not sure I like this.


1/ TimestampDifference has a dependency on gettimeofday, 
while my proposal utilizes clock_gettime. There are old discussions
that did not reach a conclusion comparing both mechanisms. 
My main conclusion from these hacker discussions [1], [2] and other 
online discussions on the topic is clock_gettime should replace
getimeofday when possible. Precision is the main reason.

2/ It no longer uses the remain time. I think the remain time
is still required here. I did a unrealistic stress test which shows 
the original proposal can handle frequent interruptions much better.

#1 in one session kicked off a vacuum

    set vacuum_cost_delay = 10;
    set vacuum_cost_limit = 1;
    set client_min_messages = log;
    update large_tbl set version = 1;
    vacuum (verbose, parallel 4) large_tbl;

#2 in another session, ran a loop to continually
interrupt the vacuum leader. This was during the
“heap scan” phase of the vacuum.

PID=< pid of vacuum leader >
while :
do
    kill -USR1 $PID
done


Using the proposed loop with the remainder, I noticed that
the actual time reported remains close to the requested
delay time.

LOG:  10.000000,10.013420
LOG:  10.000000,10.011188
LOG:  10.000000,10.010860
LOG:  10.000000,10.014839
LOG:  10.000000,10.004542
LOG:  10.000000,10.006035
LOG:  10.000000,10.012230
LOG:  10.000000,10.014535
LOG:  10.000000,10.009645
LOG:  10.000000,10.000817
LOG:  10.000000,10.002162
LOG:  10.000000,10.011721
LOG:  10.000000,10.011655

Using the approach mentioned by Nathan, there
are large differences between requested and actual time.

LOG:  10.000000,17.801778
LOG:  10.000000,12.795450
LOG:  10.000000,11.793723
LOG:  10.000000,11.796317
LOG:  10.000000,13.785993
LOG:  10.000000,11.803775
LOG:  10.000000,15.782767
LOG:  10.000000,31.783901
LOG:  10.000000,19.792440
LOG:  10.000000,21.795795
LOG:  10.000000,18.800412
LOG:  10.000000,16.782886
LOG:  10.000000,10.795197
LOG:  10.000000,14.793333
LOG:  10.000000,29.806556
LOG:  10.000000,18.810784
LOG:  10.000000,11.804956
LOG:  10.000000,24.809812
LOG:  10.000000,25.815600
LOG:  10.000000,22.809493
LOG:  10.000000,22.790908
LOG:  10.000000,19.699097
LOG:  10.000000,23.795613
LOG:  10.000000,24.797078

Let me know what you think?

[1] https://www.postgresql.org/message-id/flat/31856.1400021891%40sss.pgh.pa.us
[2] https://www.postgresql.org/message-id/flat/E1cO7fR-0003y0-9E%40gemulon.postgresql.org



Regards,

Sami 



^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:19       ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:34         ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-11 18:10           ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 19:56             ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-12 17:14               ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
@ 2024-07-12 19:18                 ` Nathan Bossart <[email protected]>
  2024-07-12 20:39                   ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  0 siblings, 1 reply; 43+ messages in thread

From: Nathan Bossart @ 2024-07-12 19:18 UTC (permalink / raw)
  To: Sami Imseih <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

On Fri, Jul 12, 2024 at 12:14:56PM -0500, Sami Imseih wrote:
> 1/ TimestampDifference has a dependency on gettimeofday, 
> while my proposal utilizes clock_gettime. There are old discussions
> that did not reach a conclusion comparing both mechanisms. 
> My main conclusion from these hacker discussions [1], [2] and other 
> online discussions on the topic is clock_gettime should replace
> getimeofday when possible. Precision is the main reason.
> 
> 2/ It no longer uses the remain time. I think the remain time
> is still required here. I did a unrealistic stress test which shows 
> the original proposal can handle frequent interruptions much better.

My comment was mostly about coding style and not about gettimeofday()
versus clock_gettime().  What does your testing show when you don't have
the extra check, i.e.,

	struct timespec delay;
	struct timespec remain;

	delay.tv_sec = microsec / 1000000L;
	delay.tv_nsec = (microsec % 1000000L) * 1000;

	while (nanosleep(&delay, &remain) == -1 && errno == EINTR)
		delay = remain;

-- 
nathan





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:19       ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:34         ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-11 18:10           ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 19:56             ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-12 17:14               ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-12 19:18                 ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
@ 2024-07-12 20:39                   ` Sami Imseih <[email protected]>
  2024-07-15 17:20                     ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  0 siblings, 1 reply; 43+ messages in thread

From: Sami Imseih @ 2024-07-12 20:39 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers


> What does your testing show when you don't have
> the extra check, i.e.,
> 
> 	struct timespec delay;
> 	struct timespec remain;
> 
> 	delay.tv_sec = microsec / 1000000L;
> 	delay.tv_nsec = (microsec % 1000000L) * 1000;
> 
> 	while (nanosleep(&delay, &remain) == -1 && errno == EINTR)
> 		delay = remain;
> 


This is similar to the first attempt [1], 

+pg_usleep_handle_interrupt(long microsec, bool force)
 {
    if (microsec > 0)
    {
 #ifndef WIN32
        struct timespec delay;
+       struct timespec remaining;
 
        delay.tv_sec = microsec / 1000000L;
        delay.tv_nsec = (microsec % 1000000L) * 1000;
-       (void) nanosleep(&delay, NULL);
+
+       if (force)
+           while (nanosleep(&delay, &remaining) == -1 && errno == EINTR)
+               delay = remaining;


but Bertrand found long drifts [2[ which I could not reproduce.
To safeguard the long drifts, continue to use the &remain time with an 
additional safeguard to make sure the actual sleep does not exceed the 
requested sleep time.

[1] https://www.postgresql.org/message-id/7D50DC5B-80C6-47B5-8DA8-A6C68A115EE5%40gmail.com
[2] https://www.postgresql.org/message-id/ZoPC5IeP4k7sZpki%40ip-10-97-1-34.eu-west-3.compute.internal


Regards,

Sami 



^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:19       ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:34         ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-11 18:10           ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 19:56             ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-12 17:14               ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-12 19:18                 ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-12 20:39                   ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
@ 2024-07-15 17:20                     ` Nathan Bossart <[email protected]>
  2024-07-16 05:06                       ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  0 siblings, 1 reply; 43+ messages in thread

From: Nathan Bossart @ 2024-07-15 17:20 UTC (permalink / raw)
  To: Sami Imseih <[email protected]>; +Cc: Bertrand Drouvot <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

On Fri, Jul 12, 2024 at 03:39:57PM -0500, Sami Imseih wrote:
> but Bertrand found long drifts [2[ which I could not reproduce.
> To safeguard the long drifts, continue to use the &remain time with an 
> additional safeguard to make sure the actual sleep does not exceed the 
> requested sleep time.

Bertrand, does this safeguard fix the long drifts you saw?

-- 
nathan





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:19       ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:34         ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-11 18:10           ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 19:56             ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-12 17:14               ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-12 19:18                 ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-12 20:39                   ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-15 17:20                     ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
@ 2024-07-16 05:06                       ` Bertrand Drouvot <[email protected]>
  2024-07-25 22:27                         ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  0 siblings, 1 reply; 43+ messages in thread

From: Bertrand Drouvot @ 2024-07-16 05:06 UTC (permalink / raw)
  To: Nathan Bossart <[email protected]>; +Cc: Sami Imseih <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

Hi,

On Mon, Jul 15, 2024 at 12:20:29PM -0500, Nathan Bossart wrote:
> On Fri, Jul 12, 2024 at 03:39:57PM -0500, Sami Imseih wrote:
> > but Bertrand found long drifts [2[ which I could not reproduce.
> > To safeguard the long drifts, continue to use the &remain time with an 
> > additional safeguard to make sure the actual sleep does not exceed the 
> > requested sleep time.
> 
> Bertrand, does this safeguard fix the long drifts you saw?

Yeah, it was the case with the first version using the safeguard (see [1]) and
it's also the case with the last one shared in [2].

[1]: https://www.postgresql.org/message-id/Zo0UdeE3i9d0Wt5E%40ip-10-97-1-34.eu-west-3.compute.internal
[2]: https://www.postgresql.org/message-id/C18017A1-EDFD-4F2F-BCA1-0572D4CCC92B%40gmail.com 

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:19       ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:34         ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-11 18:10           ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 19:56             ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-12 17:14               ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-12 19:18                 ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-12 20:39                   ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-15 17:20                     ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-16 05:06                       ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
@ 2024-07-25 22:27                         ` Sami Imseih <[email protected]>
  2024-07-25 22:29                           ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-26 08:27                           ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  0 siblings, 2 replies; 43+ messages in thread

From: Sami Imseih @ 2024-07-25 22:27 UTC (permalink / raw)
  To: Bertrand Drouvot <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

I am attaching v3 of the patch which addresses the comments made
earlier by Bertrand about the comment in the patch [1]. Also I will stick with
vacuum_sleep as the name as the function will be inside vacuum.c. I am not
sure we should make this function available outside of vacuum, but I would like
to hear other thoughts.

Also, earlier in the thread, Alvaro mentions what happens
if the sleep time is 0 [2]. In that case, we do not do anything as we check
if sleep time is > 0 microseconds before proceeding with the sleep


[1] https://www.postgresql.org/message-id/ZpDhS4nFX66ItAze%40ip-10-97-1-34.eu-west-3.compute.internal
[2] https://www.postgresql.org/message-id/202407120939.pr6wpjffmxov%40alvherre.pgsql


Regards,

Sami





Attachments:

  [application/octet-stream] v3-0001-vaccum_delay-with-absolute-time-nanosleep.patch (2.3K, ../../[email protected]/3-v3-0001-vaccum_delay-with-absolute-time-nanosleep.patch)
  download | inline diff:
From f77cf0f831df74e5e2d11910cea395f5f4541d25 Mon Sep 17 00:00:00 2001
From: Sami Imseih <[email protected]>
Date: Thu, 25 Jul 2024 20:28:35 +0000
Subject: [PATCH 1/1] vaccum_delay with absolute time nanosleep

---
 src/backend/commands/vacuum.c | 49 ++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 48f8eab202..a753d6984f 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -116,6 +116,53 @@ static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 static double compute_parallel_delay(void);
 static VacOptValue get_vacoptval_from_boolean(DefElem *def);
 static bool vac_tid_reaped(ItemPointer itemptr, void *state);
+static void vacuum_sleep(double msec);
+
+static
+void
+vacuum_sleep(double msec)
+{
+	long		microsec = msec * 1000;
+
+	if (microsec > 0)
+	{
+#ifndef WIN32
+		/*
+		 * We allow nanosleep to handle interrupts and retry with the
+		 * remaining time. However, frequent interruptions and restarts of the
+		 * nanosleep calls can substantially lead to drift in the time when
+		 * the sleep finally completes. To deal with this, we break out of the
+		 * loop whenever the elapsed time exceeds the requested time.
+		 */
+		struct timespec delay;
+		struct timespec remain;
+		struct instr_time start_time;
+
+		INSTR_TIME_SET_CURRENT(start_time);
+
+		delay.tv_sec = microsec / 1000000L;
+		delay.tv_nsec = (microsec % 1000000L) * 1000;
+
+		while (nanosleep(&delay, &remain) == -1 && errno == EINTR)
+		{
+			struct instr_time current_time;
+			struct instr_time elapsed_time;
+
+			INSTR_TIME_SET_CURRENT(current_time);
+
+			INSTR_TIME_SET_ZERO(elapsed_time);
+			INSTR_TIME_ACCUM_DIFF(elapsed_time, current_time, start_time);
+
+			if (INSTR_TIME_GET_MICROSEC(elapsed_time) >= microsec)
+				break;
+
+			delay = remain;
+		}
+#else
+		SleepEx((microsec < 500 ? 1 : (microsec + 500) / 1000), FALSE);
+#endif
+	}
+}
 
 /*
  * GUC check function to ensure GUC value specified is within the allowable
@@ -2384,7 +2431,7 @@ vacuum_delay_point(void)
 			msec = vacuum_cost_delay * 4;
 
 		pgstat_report_wait_start(WAIT_EVENT_VACUUM_DELAY);
-		pg_usleep(msec * 1000);
+		vacuum_sleep(msec);
 		pgstat_report_wait_end();
 
 		/*
-- 
2.43.0



^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:19       ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:34         ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-11 18:10           ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 19:56             ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-12 17:14               ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-12 19:18                 ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-12 20:39                   ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-15 17:20                     ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-16 05:06                       ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-25 22:27                         ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
@ 2024-07-25 22:29                           ` Sami Imseih <[email protected]>
  1 sibling, 0 replies; 43+ messages in thread

From: Sami Imseih @ 2024-07-25 22:29 UTC (permalink / raw)
  To: Bertrand Drouvot <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers



attaching the patch again. Something is strange with my email client.

Regards,

Sami

Attachments:

  [application/octet-stream] v3-0001-vaccum_delay-with-absolute-time-nanosleep.patch (2.3K, ../../[email protected]/1-v3-0001-vaccum_delay-with-absolute-time-nanosleep.patch)
  download | inline diff:
From f77cf0f831df74e5e2d11910cea395f5f4541d25 Mon Sep 17 00:00:00 2001
From: Sami Imseih <[email protected]>
Date: Thu, 25 Jul 2024 20:28:35 +0000
Subject: [PATCH 1/1] vaccum_delay with absolute time nanosleep

---
 src/backend/commands/vacuum.c | 49 ++++++++++++++++++++++++++++++++++-
 1 file changed, 48 insertions(+), 1 deletion(-)

diff --git a/src/backend/commands/vacuum.c b/src/backend/commands/vacuum.c
index 48f8eab202..a753d6984f 100644
--- a/src/backend/commands/vacuum.c
+++ b/src/backend/commands/vacuum.c
@@ -116,6 +116,53 @@ static bool vacuum_rel(Oid relid, RangeVar *relation, VacuumParams *params,
 static double compute_parallel_delay(void);
 static VacOptValue get_vacoptval_from_boolean(DefElem *def);
 static bool vac_tid_reaped(ItemPointer itemptr, void *state);
+static void vacuum_sleep(double msec);
+
+static
+void
+vacuum_sleep(double msec)
+{
+	long		microsec = msec * 1000;
+
+	if (microsec > 0)
+	{
+#ifndef WIN32
+		/*
+		 * We allow nanosleep to handle interrupts and retry with the
+		 * remaining time. However, frequent interruptions and restarts of the
+		 * nanosleep calls can substantially lead to drift in the time when
+		 * the sleep finally completes. To deal with this, we break out of the
+		 * loop whenever the elapsed time exceeds the requested time.
+		 */
+		struct timespec delay;
+		struct timespec remain;
+		struct instr_time start_time;
+
+		INSTR_TIME_SET_CURRENT(start_time);
+
+		delay.tv_sec = microsec / 1000000L;
+		delay.tv_nsec = (microsec % 1000000L) * 1000;
+
+		while (nanosleep(&delay, &remain) == -1 && errno == EINTR)
+		{
+			struct instr_time current_time;
+			struct instr_time elapsed_time;
+
+			INSTR_TIME_SET_CURRENT(current_time);
+
+			INSTR_TIME_SET_ZERO(elapsed_time);
+			INSTR_TIME_ACCUM_DIFF(elapsed_time, current_time, start_time);
+
+			if (INSTR_TIME_GET_MICROSEC(elapsed_time) >= microsec)
+				break;
+
+			delay = remain;
+		}
+#else
+		SleepEx((microsec < 500 ? 1 : (microsec + 500) / 1000), FALSE);
+#endif
+	}
+}
 
 /*
  * GUC check function to ensure GUC value specified is within the allowable
@@ -2384,7 +2431,7 @@ vacuum_delay_point(void)
 			msec = vacuum_cost_delay * 4;
 
 		pgstat_report_wait_start(WAIT_EVENT_VACUUM_DELAY);
-		pg_usleep(msec * 1000);
+		vacuum_sleep(msec);
 		pgstat_report_wait_end();
 
 		/*
-- 
2.43.0



^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:19       ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:34         ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-11 18:10           ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 19:56             ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-12 17:14               ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-12 19:18                 ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-12 20:39                   ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-15 17:20                     ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-16 05:06                       ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-25 22:27                         ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
@ 2024-07-26 08:27                           ` Bertrand Drouvot <[email protected]>
  2024-07-29 23:15                             ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  1 sibling, 1 reply; 43+ messages in thread

From: Bertrand Drouvot @ 2024-07-26 08:27 UTC (permalink / raw)
  To: Sami Imseih <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

Hi,

On Thu, Jul 25, 2024 at 05:27:15PM -0500, Sami Imseih wrote:
> I am attaching v3 of the patch which addresses the comments made
> earlier by Bertrand about the comment in the patch [1].

Thanks!

Looking at it:

1 ===

+       struct instr_time start_time;

I think we can get rid of the "struct" keyword here.

2 ===

+           struct instr_time current_time;
+           struct instr_time elapsed_time;

Same as above.

3 ===

I gave more thoughts and I think it can be simplified a bit to reduce the
number of operations in the while loop.

What about relying on a "absolute" time that way:

	instr_time absolute;
    absolute.ticks = start_time.ticks + msec * 1000000;

and then in the while loop:

    while (nanosleep(&delay, &remain) == -1 && errno == EINTR)
    {
        instr_time current_time;
        INSTR_TIME_SET_CURRENT(current_time);

        if (current_time.ticks > absolute.ticks)
        {
            break;

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:19       ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:34         ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-11 18:10           ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 19:56             ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-12 17:14               ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-12 19:18                 ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-12 20:39                   ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-15 17:20                     ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-16 05:06                       ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-25 22:27                         ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-26 08:27                           ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
@ 2024-07-29 23:15                             ` Sami Imseih <[email protected]>
  2024-07-30 06:31                               ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  0 siblings, 1 reply; 43+ messages in thread

From: Sami Imseih @ 2024-07-29 23:15 UTC (permalink / raw)
  To: Bertrand Drouvot <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers



> On Jul 26, 2024, at 3:27 AM, Bertrand Drouvot <[email protected]> wrote:
> 
> Hi,
> 
> On Thu, Jul 25, 2024 at 05:27:15PM -0500, Sami Imseih wrote:
>> I am attaching v3 of the patch which addresses the comments made
>> earlier by Bertrand about the comment in the patch [1].
> 
> Thanks!
> 
> Looking at it:
> 
> 1 ===
> 
> +       struct instr_time start_time;
> 
> I think we can get rid of the "struct" keyword here.
> 
> 2 ===
> 
> +           struct instr_time current_time;
> +           struct instr_time elapsed_time;
> 
> Same as above.

Will fix those 2.

> 
> 3 ===
> 
> I gave more thoughts and I think it can be simplified a bit to reduce the
> number of operations in the while loop.
> 
> What about relying on a "absolute" time that way:
> 
> 	instr_time absolute;
>    absolute.ticks = start_time.ticks + msec * 1000000;
> 
> and then in the while loop:
> 
>    while (nanosleep(&delay, &remain) == -1 && errno == EINTR)
>    {
>        instr_time current_time;
>        INSTR_TIME_SET_CURRENT(current_time);
> 
>        if (current_time.ticks > absolute.ticks)
>        {
>            break;

While I agree this code is cleaner, myy hesitation there is we don’t 
have any other place in which we access .ticks directly and the 
common practice is to use the intsr_time.h APIs.


What do you think?


Regards,

Sami 







^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:19       ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 15:34         ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-11 18:10           ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-11 19:56             ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-12 17:14               ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-12 19:18                 ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-12 20:39                   ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-15 17:20                     ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
  2024-07-16 05:06                       ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-25 22:27                         ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-26 08:27                           ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-29 23:15                             ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
@ 2024-07-30 06:31                               ` Bertrand Drouvot <[email protected]>
  0 siblings, 0 replies; 43+ messages in thread

From: Bertrand Drouvot @ 2024-07-30 06:31 UTC (permalink / raw)
  To: Sami Imseih <[email protected]>; +Cc: Nathan Bossart <[email protected]>; Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

Hi,

On Mon, Jul 29, 2024 at 06:15:49PM -0500, Sami Imseih wrote:
> > On Jul 26, 2024, at 3:27 AM, Bertrand Drouvot <[email protected]> wrote:
> > 3 ===
> > 
> > I gave more thoughts and I think it can be simplified a bit to reduce the
> > number of operations in the while loop.
> > 
> > What about relying on a "absolute" time that way:
> > 
> > 	instr_time absolute;
> >    absolute.ticks = start_time.ticks + msec * 1000000;
> > 
> > and then in the while loop:
> > 
> >    while (nanosleep(&delay, &remain) == -1 && errno == EINTR)
> >    {
> >        instr_time current_time;
> >        INSTR_TIME_SET_CURRENT(current_time);
> > 
> >        if (current_time.ticks > absolute.ticks)
> >        {
> >            break;
> 
> While I agree this code is cleaner, myy hesitation there is we don’t 
> have any other place in which we access .ticks directly and the 
> common practice is to use the intsr_time.h APIs.

yeah, we already have a few macros that access the .ticks, so maybe we could add
2 new ones, say:

1. INSTR_TIME_ADD_MS(t1, msec)
2. INSTR_TIME_IS_GREATER(t1, t2)

I think the less operations is done in the while loop the better.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

* Re: Restart pg_usleep when interrupted
  2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
  2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
  2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
@ 2024-07-12 07:54       ` Bertrand Drouvot <[email protected]>
  1 sibling, 0 replies; 43+ messages in thread

From: Bertrand Drouvot @ 2024-07-12 07:54 UTC (permalink / raw)
  To: Sami Imseih <[email protected]>; +Cc: Tom Lane <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers

Hi,

On Thu, Jul 11, 2024 at 10:15:41AM -0500, Sami Imseih wrote:
> 
> > I did a few tests with the patch and did not see any "large" drifts like the
> > ones observed above.
> 
> Thanks for testing.
> 
> > I think it could be "simplified" by making use of instr_time instead of timespec
> > for current and absolute. Then I think it would be enough to compare their
> > ticks.
> 
> Correct I attached a v2 of this patch that uses instr_time to check the elapsed
> time and break out of the loop. It needs some more benchmarking.

Thanks!

Outside of Nathan's comment:

1 ===

+                * However, since nanosleep is susceptible to time drift when interrupted
+                * frequently, we add a safeguard to break out of the nanosleep whenever the

I'm not sure that "nanosleep is susceptible to time drift when interrupted frequently"
is a correct wording.

What about?

"
However, since the time between frequent interruptions and restarts of the
nanosleep calls can substantially lead to drift in the time when the sleep
finally completes, we add...."

2 ===

+static void vacuum_sleep(double msec);

What about a more generic name that could be used outside of vacuum?

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com





^ permalink  raw  reply  [nested|flat] 43+ messages in thread

end of thread, other threads:[~2024-07-30 06:31 UTC | newest]

Thread overview: 43+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH 2/2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2020-11-30 19:01 [PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY Alvaro Herrera <[email protected]>
2024-07-02 09:05 Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
2024-07-05 16:49 ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
2024-07-05 16:57   ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
2024-07-09 10:44   ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
2024-07-11 15:15     ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
2024-07-11 15:19       ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
2024-07-11 15:34         ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
2024-07-11 18:10           ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
2024-07-11 19:56             ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
2024-07-12 09:39               ` Re: Restart pg_usleep when interrupted Alvaro Herrera <[email protected]>
2024-07-12 17:14               ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
2024-07-12 19:18                 ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
2024-07-12 20:39                   ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
2024-07-15 17:20                     ` Re: Restart pg_usleep when interrupted Nathan Bossart <[email protected]>
2024-07-16 05:06                       ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
2024-07-25 22:27                         ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
2024-07-25 22:29                           ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
2024-07-26 08:27                           ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
2024-07-29 23:15                             ` Re: Restart pg_usleep when interrupted Sami Imseih <[email protected]>
2024-07-30 06:31                               ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>
2024-07-12 07:54       ` Re: Restart pg_usleep when interrupted Bertrand Drouvot <[email protected]>

This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox