public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v2] set PROC_IN_SAFE_IC during REINDEX CONCURRENTLY
4+ messages / 3 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; 4+ 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] 4+ messages in thread
* Re: Improve WALRead() to suck data directly from WAL buffers when possible
@ 2023-01-14 08:48 Jeff Davis <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Jeff Davis @ 2023-01-14 08:48 UTC (permalink / raw)
To: Bharath Rupireddy <[email protected]>; Dilip Kumar <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]
On Mon, 2022-12-26 at 14:20 +0530, Bharath Rupireddy wrote:
> Please review the attached v2 patch further.
I'm still unclear on the performance goals of this patch. I see that it
will reduce syscalls, which sounds good, but to what end?
Does it allow a greater number of walsenders? Lower replication
latency? Less IO bandwidth? All of the above?
--
Jeff Davis
PostgreSQL Contributor Team - AWS
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Improve WALRead() to suck data directly from WAL buffers when possible
@ 2023-01-14 20:34 Andres Freund <[email protected]>
parent: Jeff Davis <[email protected]>
0 siblings, 1 reply; 4+ messages in thread
From: Andres Freund @ 2023-01-14 20:34 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Dilip Kumar <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]
Hi,
On 2023-01-14 00:48:52 -0800, Jeff Davis wrote:
> On Mon, 2022-12-26 at 14:20 +0530, Bharath Rupireddy wrote:
> > Please review the attached v2 patch further.
>
> I'm still unclear on the performance goals of this patch. I see that it
> will reduce syscalls, which sounds good, but to what end?
>
> Does it allow a greater number of walsenders? Lower replication
> latency? Less IO bandwidth? All of the above?
One benefit would be that it'd make it more realistic to use direct IO for WAL
- for which I have seen significant performance benefits. But when we
afterwards have to re-read it from disk to replicate, it's less clearly a win.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 4+ messages in thread
* Re: Improve WALRead() to suck data directly from WAL buffers when possible
@ 2023-01-25 21:15 Andres Freund <[email protected]>
parent: Andres Freund <[email protected]>
0 siblings, 0 replies; 4+ messages in thread
From: Andres Freund @ 2023-01-25 21:15 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Bharath Rupireddy <[email protected]>; Dilip Kumar <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; SATYANARAYANA NARLAPURAM <[email protected]>
Hi,
On 2023-01-14 12:34:03 -0800, Andres Freund wrote:
> On 2023-01-14 00:48:52 -0800, Jeff Davis wrote:
> > On Mon, 2022-12-26 at 14:20 +0530, Bharath Rupireddy wrote:
> > > Please review the attached v2 patch further.
> >
> > I'm still unclear on the performance goals of this patch. I see that it
> > will reduce syscalls, which sounds good, but to what end?
> >
> > Does it allow a greater number of walsenders? Lower replication
> > latency? Less IO bandwidth? All of the above?
>
> One benefit would be that it'd make it more realistic to use direct IO for WAL
> - for which I have seen significant performance benefits. But when we
> afterwards have to re-read it from disk to replicate, it's less clearly a win.
Satya's email just now reminded me of another important reason:
Eventually we should add the ability to stream out WAL *before* it has locally
been written out and flushed. Obviously the relevant positions would have to
be noted in the relevant message in the streaming protocol, and we couldn't
generally allow standbys to apply that data yet.
That'd allow us to significantly reduce the overhead of synchronous
replication, because instead of commonly needing to send out all the pending
WAL at commit, we'd just need to send out the updated flush position. The
reason this would lower the overhead is that:
a) The reduced amount of data to be transferred reduces latency - it's easy to
accumulate a few TCP packets worth of data even in a single small OLTP
transaction
b) The remote side can start to write out data earlier
Of course this would require additional infrastructure on the receiver
side. E.g. some persistent state indicating up to where WAL is allowed to be
applied, to avoid the standby getting ahead of th eprimary, in case the
primary crash-restarts (or has more severe issues).
With a bit of work we could perform WAL replay on standby without waiting for
the fdatasync of the received WAL - that only needs to happen when a) we need
to confirm a flush position to the primary b) when we need to write back pages
from the buffer pool (and some other things).
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 4+ messages in thread
end of thread, other threads:[~2023-01-25 21:15 UTC | newest]
Thread overview: 4+ 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]>
2023-01-14 08:48 Re: Improve WALRead() to suck data directly from WAL buffers when possible Jeff Davis <[email protected]>
2023-01-14 20:34 ` Re: Improve WALRead() to suck data directly from WAL buffers when possible Andres Freund <[email protected]>
2023-01-25 21:15 ` Re: Improve WALRead() to suck data directly from WAL buffers when possible Andres Freund <[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