public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v7 1/3] Change XLogCtl->LogwrtResult to use atomic ops
11+ messages / 5 participants
[nested] [flat]
* [PATCH v7 1/3] Change XLogCtl->LogwrtResult to use atomic ops
@ 2021-02-02 17:03 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Alvaro Herrera @ 2021-02-02 17:03 UTC (permalink / raw)
Currently, access to LogwrtResult is protected by a spinlock. This
becomes severely contended in some scenarios, such as with a largish
replication flock: walsenders all calling GetFlushRecPtr repeatedly
cause the processor heat up to the point where eggs can be fried on top.
This can be reduced to a non-problem by replacing XLogCtl->LogwrtResult
with a struct containing a pair of atomically accessed variables.
In addition, this commit splits the process-local copy of these values
(kept in the freestanding LogwrtResult struct) into two separate
LogWriteResult and LogFlushResult. This is not strictly necessary, but
it makes it clearer that these are updated separately, each on their own
schedule.
Author: Álvaro Herrera <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/access/transam/xlog.c | 85 +++++++++++++++----------------
src/include/port/atomics.h | 29 +++++++++++
src/tools/pgindent/typedefs.list | 1 +
3 files changed, 72 insertions(+), 43 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 4ac3871c74..6f2eb494fe 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -285,16 +285,13 @@ static bool doPageWrites;
*
* LogwrtRqst indicates a byte position that we need to write and/or fsync
* the log up to (all records before that point must be written or fsynced).
- * LogwrtResult indicates the byte positions we have already written/fsynced.
- * These structs are identical but are declared separately to indicate their
- * slightly different functions.
+ * LogWrtResult indicates the byte positions we have already written/fsynced.
+ * These structs are similar but are declared separately to indicate their
+ * slightly different functions; in addition, the latter is read and written
+ * using atomic operations.
*
- * To read XLogCtl->LogwrtResult, you must hold either info_lck or
- * WALWriteLock. To update it, you need to hold both locks. The point of
- * this arrangement is that the value can be examined by code that already
- * holds WALWriteLock without needing to grab info_lck as well. In addition
- * to the shared variable, each backend has a private copy of LogwrtResult,
- * which is updated when convenient.
+ * In addition to the shared variable, each backend has a private copy of
+ * LogwrtResult, each member of which is separately updated when convenient.
*
* The request bookkeeping is simpler: there is a shared XLogCtl->LogwrtRqst
* (protected by info_lck), but we don't need to cache any copies of it.
@@ -317,6 +314,12 @@ static bool doPageWrites;
*----------
*/
+typedef struct XLogwrtAtomic
+{
+ pg_atomic_uint64 Write; /* last byte + 1 written out */
+ pg_atomic_uint64 Flush; /* last byte + 1 flushed */
+} XLogwrtAtomic;
+
typedef struct XLogwrtRqst
{
XLogRecPtr Write; /* last byte + 1 to write out */
@@ -480,6 +483,7 @@ typedef struct XLogCtlData
{
XLogCtlInsert Insert;
+ XLogwrtAtomic LogwrtResult; /* uses atomics */
/* Protected by info_lck: */
XLogwrtRqst LogwrtRqst;
XLogRecPtr RedoRecPtr; /* a recent copy of Insert->RedoRecPtr */
@@ -497,12 +501,6 @@ typedef struct XLogCtlData
pg_time_t lastSegSwitchTime;
XLogRecPtr lastSegSwitchLSN;
- /*
- * Protected by info_lck and WALWriteLock (you must hold either lock to
- * read it, but both to update)
- */
- XLogwrtResult LogwrtResult;
-
/*
* Latest initialized page in the cache (last byte position + 1).
*
@@ -622,7 +620,7 @@ static ControlFileData *ControlFile = NULL;
static int UsableBytesInSegment;
/*
- * Private, possibly out-of-date copy of shared LogwrtResult.
+ * Private, possibly out-of-date copy of shared XLogCtl->LogwrtResult.
* See discussion above.
*/
static XLogwrtResult LogwrtResult = {0, 0};
@@ -932,8 +930,6 @@ XLogInsertRecord(XLogRecData *rdata,
/* advance global request to include new block(s) */
if (XLogCtl->LogwrtRqst.Write < EndPos)
XLogCtl->LogwrtRqst.Write = EndPos;
- /* update local result copy while I have the chance */
- LogwrtResult = XLogCtl->LogwrtResult;
SpinLockRelease(&XLogCtl->info_lck);
}
@@ -1811,6 +1807,7 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic)
* Now that we have the lock, check if someone initialized the page
* already.
*/
+ LogwrtResult.Write = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Write);
while (upto >= XLogCtl->InitializedUpTo || opportunistic)
{
nextidx = XLogRecPtrToBufIdx(XLogCtl->InitializedUpTo);
@@ -1830,17 +1827,17 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic)
if (opportunistic)
break;
- /* Before waiting, get info_lck and update LogwrtResult */
+ /* Advance shared memory write request position */
SpinLockAcquire(&XLogCtl->info_lck);
if (XLogCtl->LogwrtRqst.Write < OldPageRqstPtr)
XLogCtl->LogwrtRqst.Write = OldPageRqstPtr;
- LogwrtResult = XLogCtl->LogwrtResult;
SpinLockRelease(&XLogCtl->info_lck);
/*
- * Now that we have an up-to-date LogwrtResult value, see if we
- * still need to write it or if someone else already did.
+ * Before waiting, update LogwrtResult.Write and see if we still need
+ * to write it or if someone else already did.
*/
+ LogwrtResult.Write = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Write);
if (LogwrtResult.Write < OldPageRqstPtr)
{
/*
@@ -1855,7 +1852,7 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic)
LWLockAcquire(WALWriteLock, LW_EXCLUSIVE);
- LogwrtResult = XLogCtl->LogwrtResult;
+ LogwrtResult.Write = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Write);
if (LogwrtResult.Write >= OldPageRqstPtr)
{
/* OK, someone wrote it already */
@@ -2101,7 +2098,7 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
/*
* Update local LogwrtResult (caller probably did this already, but...)
*/
- LogwrtResult = XLogCtl->LogwrtResult;
+ LogwrtResult.Write = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Write);
/*
* Since successive pages in the xlog cache are consecutively allocated,
@@ -2316,6 +2313,9 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
Assert(npages == 0);
+ /* Publish current write result position */
+ pg_atomic_monotonic_advance_u64(&XLogCtl->LogwrtResult.Write, LogwrtResult.Write);
+
/*
* If asked to flush, do so
*/
@@ -2352,16 +2352,16 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
LogwrtResult.Flush = LogwrtResult.Write;
}
+ /* Publish current flush result position */
+ pg_atomic_monotonic_advance_u64(&XLogCtl->LogwrtResult.Flush, LogwrtResult.Flush);
+
/*
- * Update shared-memory status
- *
- * We make sure that the shared 'request' values do not fall behind the
+ * Make sure that the shared 'request' values do not fall behind the
* 'result' values. This is not absolutely essential, but it saves some
* code in a couple of places.
*/
{
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->LogwrtResult = LogwrtResult;
if (XLogCtl->LogwrtRqst.Write < LogwrtResult.Write)
XLogCtl->LogwrtRqst.Write = LogwrtResult.Write;
if (XLogCtl->LogwrtRqst.Flush < LogwrtResult.Flush)
@@ -2381,8 +2381,8 @@ XLogSetAsyncXactLSN(XLogRecPtr asyncXactLSN)
XLogRecPtr WriteRqstPtr = asyncXactLSN;
bool sleeping;
+ LogwrtResult.Flush = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Flush);
SpinLockAcquire(&XLogCtl->info_lck);
- LogwrtResult = XLogCtl->LogwrtResult;
sleeping = XLogCtl->WalWriterSleeping;
if (XLogCtl->asyncXactLSN < asyncXactLSN)
XLogCtl->asyncXactLSN = asyncXactLSN;
@@ -2587,10 +2587,10 @@ XLogFlush(XLogRecPtr record)
SpinLockAcquire(&XLogCtl->info_lck);
if (WriteRqstPtr < XLogCtl->LogwrtRqst.Write)
WriteRqstPtr = XLogCtl->LogwrtRqst.Write;
- LogwrtResult = XLogCtl->LogwrtResult;
SpinLockRelease(&XLogCtl->info_lck);
/* done already? */
+ LogwrtResult.Flush = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Flush);
if (record <= LogwrtResult.Flush)
break;
@@ -2618,7 +2618,7 @@ XLogFlush(XLogRecPtr record)
}
/* Got the lock; recheck whether request is satisfied */
- LogwrtResult = XLogCtl->LogwrtResult;
+ LogwrtResult.Flush = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Flush);
if (record <= LogwrtResult.Flush)
{
LWLockRelease(WALWriteLock);
@@ -2742,7 +2742,6 @@ XLogBackgroundFlush(void)
/* read LogwrtResult and update local state */
SpinLockAcquire(&XLogCtl->info_lck);
- LogwrtResult = XLogCtl->LogwrtResult;
WriteRqst = XLogCtl->LogwrtRqst;
SpinLockRelease(&XLogCtl->info_lck);
@@ -2750,8 +2749,10 @@ XLogBackgroundFlush(void)
WriteRqst.Write -= WriteRqst.Write % XLOG_BLCKSZ;
/* if we have already flushed that far, consider async commit records */
+ LogwrtResult.Flush = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Flush);
if (WriteRqst.Write <= LogwrtResult.Flush)
{
+ pg_memory_barrier();
SpinLockAcquire(&XLogCtl->info_lck);
WriteRqst.Write = XLogCtl->asyncXactLSN;
SpinLockRelease(&XLogCtl->info_lck);
@@ -2767,6 +2768,7 @@ XLogBackgroundFlush(void)
{
if (openLogFile >= 0)
{
+ LogwrtResult.Write = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Write);
if (!XLByteInPrevSeg(LogwrtResult.Write, openLogSegNo,
wal_segment_size))
{
@@ -2826,7 +2828,8 @@ XLogBackgroundFlush(void)
/* now wait for any in-progress insertions to finish and get write lock */
WaitXLogInsertionsToFinish(WriteRqst.Write);
LWLockAcquire(WALWriteLock, LW_EXCLUSIVE);
- LogwrtResult = XLogCtl->LogwrtResult;
+ LogwrtResult.Write = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Write);
+ LogwrtResult.Flush = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Flush);
if (WriteRqst.Write > LogwrtResult.Write ||
WriteRqst.Flush > LogwrtResult.Flush)
{
@@ -2914,9 +2917,7 @@ XLogNeedsFlush(XLogRecPtr record)
return false;
/* read LogwrtResult and update local state */
- SpinLockAcquire(&XLogCtl->info_lck);
- LogwrtResult = XLogCtl->LogwrtResult;
- SpinLockRelease(&XLogCtl->info_lck);
+ LogwrtResult.Flush = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Flush);
/* check again */
if (record <= LogwrtResult.Flush)
@@ -5489,7 +5490,9 @@ StartupXLOG(void)
LogwrtResult.Write = LogwrtResult.Flush = EndOfLog;
- XLogCtl->LogwrtResult = LogwrtResult;
+ /* XXX OK to write without WALWriteLock? */
+ pg_atomic_write_u64(&XLogCtl->LogwrtResult.Write, EndOfLog);
+ pg_atomic_write_u64(&XLogCtl->LogwrtResult.Flush, EndOfLog);
XLogCtl->LogwrtRqst.Write = EndOfLog;
XLogCtl->LogwrtRqst.Flush = EndOfLog;
@@ -5925,9 +5928,7 @@ GetFlushRecPtr(TimeLineID *insertTLI)
{
Assert(XLogCtl->SharedRecoveryState == RECOVERY_STATE_DONE);
- SpinLockAcquire(&XLogCtl->info_lck);
- LogwrtResult = XLogCtl->LogwrtResult;
- SpinLockRelease(&XLogCtl->info_lck);
+ LogwrtResult.Flush = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Flush);
/*
* If we're writing and flushing WAL, the time line can't be changing, so
@@ -9083,9 +9084,7 @@ GetXLogInsertRecPtr(void)
XLogRecPtr
GetXLogWriteRecPtr(void)
{
- SpinLockAcquire(&XLogCtl->info_lck);
- LogwrtResult = XLogCtl->LogwrtResult;
- SpinLockRelease(&XLogCtl->info_lck);
+ LogwrtResult.Write = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Write);
return LogwrtResult.Write;
}
diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
index 9550e04aaa..7abed1785a 100644
--- a/src/include/port/atomics.h
+++ b/src/include/port/atomics.h
@@ -519,6 +519,35 @@ pg_atomic_sub_fetch_u64(volatile pg_atomic_uint64 *ptr, int64 sub_)
return pg_atomic_sub_fetch_u64_impl(ptr, sub_);
}
+/*
+ * Monotonically advance the given variable using only atomic operations until
+ * it's at least the target value.
+ *
+ * Full barrier semantics (even when value is unchanged).
+ */
+static inline void
+pg_atomic_monotonic_advance_u64(volatile pg_atomic_uint64 *ptr, uint64 target_)
+{
+ uint64 currval;
+
+#ifndef PG_HAVE_ATOMIC_U64_SIMULATION
+ AssertPointerAlignment(ptr, 8);
+#endif
+
+ currval = pg_atomic_read_u64(ptr);
+ if (currval >= target_)
+ {
+ pg_memory_barrier();
+ return;
+ }
+
+ while (currval < target_)
+ {
+ if (pg_atomic_compare_exchange_u64(ptr, &currval, target_))
+ break;
+ }
+}
+
#undef INSIDE_ATOMICS_H
#endif /* ATOMICS_H */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 93d5190508..5fed02537a 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2959,6 +2959,7 @@ XLogRecoveryCtlData
XLogRedoAction
XLogSegNo
XLogSource
+XLogwrtAtomic
XLogwrtResult
XLogwrtRqst
XPVIV
--
2.30.2
--3eusckdkvtrwfp2n
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
filename="v7-0002-Split-LogwrtResult-into-separate-variables.patch"
^ permalink raw reply [nested|flat] 11+ messages in thread
* [PATCH v6 1/2] Change XLogCtl->LogwrtResult to use atomic ops
@ 2021-02-02 17:03 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Alvaro Herrera @ 2021-02-02 17:03 UTC (permalink / raw)
Currently, access to LogwrtResult is protected by a spinlock. This
becomes severely contended in some scenarios, such as with a largish
replication flock: walsenders all calling GetFlushRecPtr repeatedly
cause the processor heat up to the point where eggs can be fried on top.
This can be reduced to a non-problem by replacing XLogCtl->LogwrtResult
with a struct containing a pair of atomically accessed variables.
In addition, this commit splits the process-local copy of these values
(kept in the freestanding LogwrtResult struct) into two separate
LogWriteResult and LogFlushResult. This is not strictly necessary, but
it makes it clearer that these are updated separately, each on their own
schedule.
Author: Álvaro Herrera <[email protected]>
Discussion: https://postgr.es/m/[email protected]
---
src/backend/access/transam/xlog.c | 195 +++++++++++++++---------------
src/include/port/atomics.h | 29 +++++
src/tools/pgindent/typedefs.list | 1 +
3 files changed, 125 insertions(+), 100 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 4ac3871c74..311a8a1192 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -285,16 +285,14 @@ static bool doPageWrites;
*
* LogwrtRqst indicates a byte position that we need to write and/or fsync
* the log up to (all records before that point must be written or fsynced).
- * LogwrtResult indicates the byte positions we have already written/fsynced.
- * These structs are identical but are declared separately to indicate their
- * slightly different functions.
+ * LogWrtResult indicates the byte positions we have already written/fsynced.
+ * These structs are similar but are declared separately to indicate their
+ * slightly different functions; in addition, the latter is read and written
+ * using atomic operations.
*
- * To read XLogCtl->LogwrtResult, you must hold either info_lck or
- * WALWriteLock. To update it, you need to hold both locks. The point of
- * this arrangement is that the value can be examined by code that already
- * holds WALWriteLock without needing to grab info_lck as well. In addition
- * to the shared variable, each backend has a private copy of LogwrtResult,
- * which is updated when convenient.
+ * In addition to the shared variable, each backend has a private copy of
+ * each member of LogwrtResult (LogWriteResult and LogFlushResult), each of
+ * which is separately updated when convenient.
*
* The request bookkeeping is simpler: there is a shared XLogCtl->LogwrtRqst
* (protected by info_lck), but we don't need to cache any copies of it.
@@ -317,18 +315,18 @@ static bool doPageWrites;
*----------
*/
+typedef struct XLogwrtAtomic
+{
+ pg_atomic_uint64 Write; /* last byte + 1 of write position */
+ pg_atomic_uint64 Flush; /* last byte + 1 of flush position */
+} XLogwrtAtomic;
+
typedef struct XLogwrtRqst
{
XLogRecPtr Write; /* last byte + 1 to write out */
XLogRecPtr Flush; /* last byte + 1 to flush */
} XLogwrtRqst;
-typedef struct XLogwrtResult
-{
- XLogRecPtr Write; /* last byte + 1 written out */
- XLogRecPtr Flush; /* last byte + 1 flushed */
-} XLogwrtResult;
-
/*
* Inserting to WAL is protected by a small fixed number of WAL insertion
* locks. To insert to the WAL, you must hold one of the locks - it doesn't
@@ -480,6 +478,7 @@ typedef struct XLogCtlData
{
XLogCtlInsert Insert;
+ XLogwrtAtomic LogwrtResult; /* uses atomics */
/* Protected by info_lck: */
XLogwrtRqst LogwrtRqst;
XLogRecPtr RedoRecPtr; /* a recent copy of Insert->RedoRecPtr */
@@ -497,12 +496,6 @@ typedef struct XLogCtlData
pg_time_t lastSegSwitchTime;
XLogRecPtr lastSegSwitchLSN;
- /*
- * Protected by info_lck and WALWriteLock (you must hold either lock to
- * read it, but both to update)
- */
- XLogwrtResult LogwrtResult;
-
/*
* Latest initialized page in the cache (last byte position + 1).
*
@@ -622,10 +615,11 @@ static ControlFileData *ControlFile = NULL;
static int UsableBytesInSegment;
/*
- * Private, possibly out-of-date copy of shared LogwrtResult.
+ * Private, possibly out-of-date copy of shared XLogCtl->LogwrtResult.
* See discussion above.
*/
-static XLogwrtResult LogwrtResult = {0, 0};
+static XLogRecPtr LogWriteResult = 0;
+static XLogRecPtr LogFlushResult = 0;
/*
* openLogFile is -1 or a kernel FD for an open log file segment.
@@ -932,8 +926,6 @@ XLogInsertRecord(XLogRecData *rdata,
/* advance global request to include new block(s) */
if (XLogCtl->LogwrtRqst.Write < EndPos)
XLogCtl->LogwrtRqst.Write = EndPos;
- /* update local result copy while I have the chance */
- LogwrtResult = XLogCtl->LogwrtResult;
SpinLockRelease(&XLogCtl->info_lck);
}
@@ -1811,6 +1803,7 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic)
* Now that we have the lock, check if someone initialized the page
* already.
*/
+ LogWriteResult = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Write);
while (upto >= XLogCtl->InitializedUpTo || opportunistic)
{
nextidx = XLogRecPtrToBufIdx(XLogCtl->InitializedUpTo);
@@ -1821,7 +1814,7 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic)
* already written out.
*/
OldPageRqstPtr = XLogCtl->xlblocks[nextidx];
- if (LogwrtResult.Write < OldPageRqstPtr)
+ if (LogWriteResult < OldPageRqstPtr)
{
/*
* Nope, got work to do. If we just want to pre-initialize as much
@@ -1830,18 +1823,18 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic)
if (opportunistic)
break;
- /* Before waiting, get info_lck and update LogwrtResult */
+ /* Advance shared memory write request position */
SpinLockAcquire(&XLogCtl->info_lck);
if (XLogCtl->LogwrtRqst.Write < OldPageRqstPtr)
XLogCtl->LogwrtRqst.Write = OldPageRqstPtr;
- LogwrtResult = XLogCtl->LogwrtResult;
SpinLockRelease(&XLogCtl->info_lck);
/*
- * Now that we have an up-to-date LogwrtResult value, see if we
- * still need to write it or if someone else already did.
+ * Before waiting, update LogWriteResult and see if we still need
+ * to write it or if someone else already did.
*/
- if (LogwrtResult.Write < OldPageRqstPtr)
+ LogWriteResult = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Write);
+ if (LogWriteResult < OldPageRqstPtr)
{
/*
* Must acquire write lock. Release WALBufMappingLock first,
@@ -1855,8 +1848,8 @@ AdvanceXLInsertBuffer(XLogRecPtr upto, TimeLineID tli, bool opportunistic)
LWLockAcquire(WALWriteLock, LW_EXCLUSIVE);
- LogwrtResult = XLogCtl->LogwrtResult;
- if (LogwrtResult.Write >= OldPageRqstPtr)
+ LogWriteResult = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Write);
+ if (LogWriteResult >= OldPageRqstPtr)
{
/* OK, someone wrote it already */
LWLockRelease(WALWriteLock);
@@ -2101,7 +2094,7 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
/*
* Update local LogwrtResult (caller probably did this already, but...)
*/
- LogwrtResult = XLogCtl->LogwrtResult;
+ LogWriteResult = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Write);
/*
* Since successive pages in the xlog cache are consecutively allocated,
@@ -2121,9 +2114,9 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
* consider writing. Begin at the buffer containing the next unwritten
* page, or last partially written page.
*/
- curridx = XLogRecPtrToBufIdx(LogwrtResult.Write);
+ curridx = XLogRecPtrToBufIdx(LogWriteResult);
- while (LogwrtResult.Write < WriteRqst.Write)
+ while (LogWriteResult < WriteRqst.Write)
{
/*
* Make sure we're not ahead of the insert process. This could happen
@@ -2132,16 +2125,16 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
*/
XLogRecPtr EndPtr = XLogCtl->xlblocks[curridx];
- if (LogwrtResult.Write >= EndPtr)
+ if (LogWriteResult >= EndPtr)
elog(PANIC, "xlog write request %X/%X is past end of log %X/%X",
- LSN_FORMAT_ARGS(LogwrtResult.Write),
+ LSN_FORMAT_ARGS(LogWriteResult),
LSN_FORMAT_ARGS(EndPtr));
- /* Advance LogwrtResult.Write to end of current buffer page */
- LogwrtResult.Write = EndPtr;
- ispartialpage = WriteRqst.Write < LogwrtResult.Write;
+ /* Advance LogWriteResult to end of current buffer page */
+ LogWriteResult = EndPtr;
+ ispartialpage = WriteRqst.Write < LogWriteResult;
- if (!XLByteInPrevSeg(LogwrtResult.Write, openLogSegNo,
+ if (!XLByteInPrevSeg(LogWriteResult, openLogSegNo,
wal_segment_size))
{
/*
@@ -2151,7 +2144,7 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
Assert(npages == 0);
if (openLogFile >= 0)
XLogFileClose();
- XLByteToPrevSeg(LogwrtResult.Write, openLogSegNo,
+ XLByteToPrevSeg(LogWriteResult, openLogSegNo,
wal_segment_size);
openLogTLI = tli;
@@ -2163,7 +2156,7 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
/* Make sure we have the current logfile open */
if (openLogFile < 0)
{
- XLByteToPrevSeg(LogwrtResult.Write, openLogSegNo,
+ XLByteToPrevSeg(LogWriteResult, openLogSegNo,
wal_segment_size);
openLogTLI = tli;
openLogFile = XLogFileOpen(openLogSegNo, tli);
@@ -2175,7 +2168,7 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
{
/* first of group */
startidx = curridx;
- startoffset = XLogSegmentOffset(LogwrtResult.Write - XLOG_BLCKSZ,
+ startoffset = XLogSegmentOffset(LogWriteResult - XLOG_BLCKSZ,
wal_segment_size);
}
npages++;
@@ -2186,7 +2179,7 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
* contiguous in memory), or if we are at the end of the logfile
* segment.
*/
- last_iteration = WriteRqst.Write <= LogwrtResult.Write;
+ last_iteration = WriteRqst.Write <= LogWriteResult;
finishing_seg = !ispartialpage &&
(startoffset + npages * XLOG_BLCKSZ) >= wal_segment_size;
@@ -2277,13 +2270,13 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
/* signal that we need to wakeup walsenders later */
WalSndWakeupRequest();
- LogwrtResult.Flush = LogwrtResult.Write; /* end of page */
+ LogFlushResult = LogWriteResult; /* end of page */
if (XLogArchivingActive())
XLogArchiveNotifySeg(openLogSegNo, tli);
XLogCtl->lastSegSwitchTime = (pg_time_t) time(NULL);
- XLogCtl->lastSegSwitchLSN = LogwrtResult.Flush;
+ XLogCtl->lastSegSwitchLSN = LogFlushResult;
/*
* Request a checkpoint if we've consumed too much xlog since
@@ -2304,7 +2297,7 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
if (ispartialpage)
{
/* Only asked to write a partial page */
- LogwrtResult.Write = WriteRqst.Write;
+ LogWriteResult = WriteRqst.Write;
break;
}
curridx = NextBufIdx(curridx);
@@ -2316,11 +2309,14 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
Assert(npages == 0);
+ /* Publish current write result position */
+ pg_atomic_monotonic_advance_u64(&XLogCtl->LogwrtResult.Write, LogWriteResult);
+
/*
* If asked to flush, do so
*/
- if (LogwrtResult.Flush < WriteRqst.Flush &&
- LogwrtResult.Flush < LogwrtResult.Write)
+ if (LogFlushResult < WriteRqst.Flush &&
+ LogFlushResult < LogWriteResult)
{
/*
* Could get here without iterating above loop, in which case we might
@@ -2331,12 +2327,12 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
sync_method != SYNC_METHOD_OPEN_DSYNC)
{
if (openLogFile >= 0 &&
- !XLByteInPrevSeg(LogwrtResult.Write, openLogSegNo,
+ !XLByteInPrevSeg(LogWriteResult, openLogSegNo,
wal_segment_size))
XLogFileClose();
if (openLogFile < 0)
{
- XLByteToPrevSeg(LogwrtResult.Write, openLogSegNo,
+ XLByteToPrevSeg(LogWriteResult, openLogSegNo,
wal_segment_size);
openLogTLI = tli;
openLogFile = XLogFileOpen(openLogSegNo, tli);
@@ -2349,23 +2345,23 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
/* signal that we need to wakeup walsenders later */
WalSndWakeupRequest();
- LogwrtResult.Flush = LogwrtResult.Write;
+ LogFlushResult = LogWriteResult;
}
+ /* Publish current flush result position */
+ pg_atomic_monotonic_advance_u64(&XLogCtl->LogwrtResult.Flush, LogFlushResult);
+
/*
- * Update shared-memory status
- *
- * We make sure that the shared 'request' values do not fall behind the
+ * Make sure that the shared 'request' values do not fall behind the
* 'result' values. This is not absolutely essential, but it saves some
* code in a couple of places.
*/
{
SpinLockAcquire(&XLogCtl->info_lck);
- XLogCtl->LogwrtResult = LogwrtResult;
- if (XLogCtl->LogwrtRqst.Write < LogwrtResult.Write)
- XLogCtl->LogwrtRqst.Write = LogwrtResult.Write;
- if (XLogCtl->LogwrtRqst.Flush < LogwrtResult.Flush)
- XLogCtl->LogwrtRqst.Flush = LogwrtResult.Flush;
+ if (XLogCtl->LogwrtRqst.Write < LogWriteResult)
+ XLogCtl->LogwrtRqst.Write = LogWriteResult;
+ if (XLogCtl->LogwrtRqst.Flush < LogFlushResult)
+ XLogCtl->LogwrtRqst.Flush = LogFlushResult;
SpinLockRelease(&XLogCtl->info_lck);
}
}
@@ -2381,8 +2377,8 @@ XLogSetAsyncXactLSN(XLogRecPtr asyncXactLSN)
XLogRecPtr WriteRqstPtr = asyncXactLSN;
bool sleeping;
+ LogFlushResult = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Flush);
SpinLockAcquire(&XLogCtl->info_lck);
- LogwrtResult = XLogCtl->LogwrtResult;
sleeping = XLogCtl->WalWriterSleeping;
if (XLogCtl->asyncXactLSN < asyncXactLSN)
XLogCtl->asyncXactLSN = asyncXactLSN;
@@ -2399,7 +2395,7 @@ XLogSetAsyncXactLSN(XLogRecPtr asyncXactLSN)
WriteRqstPtr -= WriteRqstPtr % XLOG_BLCKSZ;
/* if we have already flushed that far, we're done */
- if (WriteRqstPtr <= LogwrtResult.Flush)
+ if (WriteRqstPtr <= LogFlushResult)
return;
}
@@ -2551,15 +2547,15 @@ XLogFlush(XLogRecPtr record)
}
/* Quick exit if already known flushed */
- if (record <= LogwrtResult.Flush)
+ if (record <= LogFlushResult)
return;
#ifdef WAL_DEBUG
if (XLOG_DEBUG)
elog(LOG, "xlog flush request %X/%X; write %X/%X; flush %X/%X",
LSN_FORMAT_ARGS(record),
- LSN_FORMAT_ARGS(LogwrtResult.Write),
- LSN_FORMAT_ARGS(LogwrtResult.Flush));
+ LSN_FORMAT_ARGS(LogWriteResult),
+ LSN_FORMAT_ARGS(LogFlushResult));
#endif
START_CRIT_SECTION();
@@ -2568,8 +2564,8 @@ XLogFlush(XLogRecPtr record)
* Since fsync is usually a horribly expensive operation, we try to
* piggyback as much data as we can on each fsync: if we see any more data
* entered into the xlog buffer, we'll write and fsync that too, so that
- * the final value of LogwrtResult.Flush is as large as possible. This
- * gives us some chance of avoiding another fsync immediately after.
+ * the final value of LogFlushResult is as large as possible. This gives
+ * us some chance of avoiding another fsync immediately after.
*/
/* initialize to given target; may increase below */
@@ -2587,11 +2583,11 @@ XLogFlush(XLogRecPtr record)
SpinLockAcquire(&XLogCtl->info_lck);
if (WriteRqstPtr < XLogCtl->LogwrtRqst.Write)
WriteRqstPtr = XLogCtl->LogwrtRqst.Write;
- LogwrtResult = XLogCtl->LogwrtResult;
SpinLockRelease(&XLogCtl->info_lck);
/* done already? */
- if (record <= LogwrtResult.Flush)
+ LogFlushResult = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Flush);
+ if (record <= LogFlushResult)
break;
/*
@@ -2618,8 +2614,8 @@ XLogFlush(XLogRecPtr record)
}
/* Got the lock; recheck whether request is satisfied */
- LogwrtResult = XLogCtl->LogwrtResult;
- if (record <= LogwrtResult.Flush)
+ LogFlushResult = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Flush);
+ if (record <= LogFlushResult)
{
LWLockRelease(WALWriteLock);
break;
@@ -2689,11 +2685,11 @@ XLogFlush(XLogRecPtr record)
* calls from bufmgr.c are not within critical sections and so we will not
* force a restart for a bad LSN on a data page.
*/
- if (LogwrtResult.Flush < record)
+ if (LogFlushResult < record)
elog(ERROR,
"xlog flush request %X/%X is not satisfied --- flushed only to %X/%X",
LSN_FORMAT_ARGS(record),
- LSN_FORMAT_ARGS(LogwrtResult.Flush));
+ LSN_FORMAT_ARGS(LogFlushResult));
}
/*
@@ -2742,7 +2738,6 @@ XLogBackgroundFlush(void)
/* read LogwrtResult and update local state */
SpinLockAcquire(&XLogCtl->info_lck);
- LogwrtResult = XLogCtl->LogwrtResult;
WriteRqst = XLogCtl->LogwrtRqst;
SpinLockRelease(&XLogCtl->info_lck);
@@ -2750,8 +2745,10 @@ XLogBackgroundFlush(void)
WriteRqst.Write -= WriteRqst.Write % XLOG_BLCKSZ;
/* if we have already flushed that far, consider async commit records */
- if (WriteRqst.Write <= LogwrtResult.Flush)
+ LogFlushResult = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Flush);
+ if (WriteRqst.Write <= LogFlushResult)
{
+ pg_memory_barrier();
SpinLockAcquire(&XLogCtl->info_lck);
WriteRqst.Write = XLogCtl->asyncXactLSN;
SpinLockRelease(&XLogCtl->info_lck);
@@ -2763,11 +2760,12 @@ XLogBackgroundFlush(void)
* holding an open file handle to a logfile that's no longer in use,
* preventing the file from being deleted.
*/
- if (WriteRqst.Write <= LogwrtResult.Flush)
+ if (WriteRqst.Write <= LogFlushResult)
{
if (openLogFile >= 0)
{
- if (!XLByteInPrevSeg(LogwrtResult.Write, openLogSegNo,
+ LogWriteResult = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Write);
+ if (!XLByteInPrevSeg(LogWriteResult, openLogSegNo,
wal_segment_size))
{
XLogFileClose();
@@ -2782,7 +2780,7 @@ XLogBackgroundFlush(void)
*/
now = GetCurrentTimestamp();
flushbytes =
- WriteRqst.Write / XLOG_BLCKSZ - LogwrtResult.Flush / XLOG_BLCKSZ;
+ WriteRqst.Write / XLOG_BLCKSZ - LogFlushResult / XLOG_BLCKSZ;
if (WalWriterFlushAfter == 0 || lastflush == 0)
{
@@ -2817,8 +2815,8 @@ XLogBackgroundFlush(void)
elog(LOG, "xlog bg flush request write %X/%X; flush: %X/%X, current is write %X/%X; flush %X/%X",
LSN_FORMAT_ARGS(WriteRqst.Write),
LSN_FORMAT_ARGS(WriteRqst.Flush),
- LSN_FORMAT_ARGS(LogwrtResult.Write),
- LSN_FORMAT_ARGS(LogwrtResult.Flush));
+ LSN_FORMAT_ARGS(LogWriteResult),
+ LSN_FORMAT_ARGS(LogFlushResult));
#endif
START_CRIT_SECTION();
@@ -2826,9 +2824,10 @@ XLogBackgroundFlush(void)
/* now wait for any in-progress insertions to finish and get write lock */
WaitXLogInsertionsToFinish(WriteRqst.Write);
LWLockAcquire(WALWriteLock, LW_EXCLUSIVE);
- LogwrtResult = XLogCtl->LogwrtResult;
- if (WriteRqst.Write > LogwrtResult.Write ||
- WriteRqst.Flush > LogwrtResult.Flush)
+ LogWriteResult = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Write);
+ LogFlushResult = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Flush);
+ if (WriteRqst.Write > LogWriteResult ||
+ WriteRqst.Flush > LogFlushResult)
{
XLogWrite(WriteRqst, insertTLI, flexible);
}
@@ -2910,16 +2909,14 @@ XLogNeedsFlush(XLogRecPtr record)
}
/* Quick exit if already known flushed */
- if (record <= LogwrtResult.Flush)
+ if (record <= LogFlushResult)
return false;
/* read LogwrtResult and update local state */
- SpinLockAcquire(&XLogCtl->info_lck);
- LogwrtResult = XLogCtl->LogwrtResult;
- SpinLockRelease(&XLogCtl->info_lck);
+ LogFlushResult = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Flush);
/* check again */
- if (record <= LogwrtResult.Flush)
+ if (record <= LogFlushResult)
return false;
return true;
@@ -5487,9 +5484,11 @@ StartupXLOG(void)
XLogCtl->InitializedUpTo = EndOfLog;
}
- LogwrtResult.Write = LogwrtResult.Flush = EndOfLog;
+ LogWriteResult = LogFlushResult = EndOfLog;
- XLogCtl->LogwrtResult = LogwrtResult;
+ /* XXX OK to write without WALWriteLock? */
+ pg_atomic_write_u64(&XLogCtl->LogwrtResult.Write, EndOfLog);
+ pg_atomic_write_u64(&XLogCtl->LogwrtResult.Flush, EndOfLog);
XLogCtl->LogwrtRqst.Write = EndOfLog;
XLogCtl->LogwrtRqst.Flush = EndOfLog;
@@ -5925,9 +5924,7 @@ GetFlushRecPtr(TimeLineID *insertTLI)
{
Assert(XLogCtl->SharedRecoveryState == RECOVERY_STATE_DONE);
- SpinLockAcquire(&XLogCtl->info_lck);
- LogwrtResult = XLogCtl->LogwrtResult;
- SpinLockRelease(&XLogCtl->info_lck);
+ LogFlushResult = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Flush);
/*
* If we're writing and flushing WAL, the time line can't be changing, so
@@ -5936,7 +5933,7 @@ GetFlushRecPtr(TimeLineID *insertTLI)
if (insertTLI)
*insertTLI = XLogCtl->InsertTimeLineID;
- return LogwrtResult.Flush;
+ return LogFlushResult;
}
/*
@@ -9083,11 +9080,9 @@ GetXLogInsertRecPtr(void)
XLogRecPtr
GetXLogWriteRecPtr(void)
{
- SpinLockAcquire(&XLogCtl->info_lck);
- LogwrtResult = XLogCtl->LogwrtResult;
- SpinLockRelease(&XLogCtl->info_lck);
+ LogWriteResult = pg_atomic_read_u64(&XLogCtl->LogwrtResult.Write);
- return LogwrtResult.Write;
+ return LogWriteResult;
}
/*
diff --git a/src/include/port/atomics.h b/src/include/port/atomics.h
index 9550e04aaa..7abed1785a 100644
--- a/src/include/port/atomics.h
+++ b/src/include/port/atomics.h
@@ -519,6 +519,35 @@ pg_atomic_sub_fetch_u64(volatile pg_atomic_uint64 *ptr, int64 sub_)
return pg_atomic_sub_fetch_u64_impl(ptr, sub_);
}
+/*
+ * Monotonically advance the given variable using only atomic operations until
+ * it's at least the target value.
+ *
+ * Full barrier semantics (even when value is unchanged).
+ */
+static inline void
+pg_atomic_monotonic_advance_u64(volatile pg_atomic_uint64 *ptr, uint64 target_)
+{
+ uint64 currval;
+
+#ifndef PG_HAVE_ATOMIC_U64_SIMULATION
+ AssertPointerAlignment(ptr, 8);
+#endif
+
+ currval = pg_atomic_read_u64(ptr);
+ if (currval >= target_)
+ {
+ pg_memory_barrier();
+ return;
+ }
+
+ while (currval < target_)
+ {
+ if (pg_atomic_compare_exchange_u64(ptr, &currval, target_))
+ break;
+ }
+}
+
#undef INSIDE_ATOMICS_H
#endif /* ATOMICS_H */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 93d5190508..5fed02537a 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2959,6 +2959,7 @@ XLogRecoveryCtlData
XLogRedoAction
XLogSegNo
XLogSource
+XLogwrtAtomic
XLogwrtResult
XLogwrtRqst
XPVIV
--
2.30.2
--rzlr6nxatuf6nv5i
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
filename="v6-0002-Change-asyncXactLSN-and-WalWriterSleeping-to-atom.patch"
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Move defaults toward ICU in 16?
@ 2023-02-17 08:06 Jeff Davis <[email protected]>
2023-02-17 17:01 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Jeff Davis @ 2023-02-17 08:06 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
On Tue, 2023-02-14 at 09:59 -0800, Andres Freund wrote:
> I am saying that pg_upgrade should be able to deal with the
> difference. The
> details of how to implement that, don't matter that much.
To clarify, you're saying that pg_upgrade should simply update
pg_database to set the new databases' collation fields equal to that of
the old cluster?
I'll submit it as a separate patch because it would be independently
useful.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Move defaults toward ICU in 16?
2023-02-17 08:06 Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
@ 2023-02-17 17:01 ` Jeff Davis <[email protected]>
2023-02-17 17:05 ` Re: Move defaults toward ICU in 16? Andres Freund <[email protected]>
2023-02-17 18:41 ` Re: Move defaults toward ICU in 16? Justin Pryzby <[email protected]>
2023-02-18 08:38 ` Re: Move defaults toward ICU in 16? Robert Haas <[email protected]>
0 siblings, 3 replies; 11+ messages in thread
From: Jeff Davis @ 2023-02-17 17:01 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
On Fri, 2023-02-17 at 00:06 -0800, Jeff Davis wrote:
> On Tue, 2023-02-14 at 09:59 -0800, Andres Freund wrote:
> > I am saying that pg_upgrade should be able to deal with the
> > difference. The
> > details of how to implement that, don't matter that much.
>
> To clarify, you're saying that pg_upgrade should simply update
> pg_database to set the new databases' collation fields equal to that
> of
> the old cluster?
Thinking about this more, it's not clear to me if this would be in
scope for pg_upgrade or not. If pg_upgrade is fixing up the new cluster
rather than checking for compatibility, why doesn't it just take over
and do the initdb for the new cluster itself? That would be less
confusing for users, and avoid some weirdness (like, if you drop the
database "postgres" on the original, why does it reappear after an
upgrade?).
Someone might want to do something interesting to the new cluster
before the upgrade, but it's not clear from the docs what would be both
useful and safe.
Regards,
Jeff Davis
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Move defaults toward ICU in 16?
2023-02-17 08:06 Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 17:01 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
@ 2023-02-17 17:05 ` Andres Freund <[email protected]>
2023-02-17 18:00 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2 siblings, 1 reply; 11+ messages in thread
From: Andres Freund @ 2023-02-17 17:05 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
Hi,
On 2023-02-17 09:01:54 -0800, Jeff Davis wrote:
> On Fri, 2023-02-17 at 00:06 -0800, Jeff Davis wrote:
> > On Tue, 2023-02-14 at 09:59 -0800, Andres Freund wrote:
> > > I am saying that pg_upgrade should be able to deal with the
> > > difference. The
> > > details of how to implement that, don't matter that much.
> >
> > To clarify, you're saying that pg_upgrade should simply update
> > pg_database to set the new databases' collation fields equal to that
> > of
> > the old cluster?
Yes.
> Thinking about this more, it's not clear to me if this would be in
> scope for pg_upgrade or not.
I don't think we should consider changing the default collation provider
without making this more seamless, one way or another.
> If pg_upgrade is fixing up the new cluster rather than checking for
> compatibility, why doesn't it just take over and do the initdb for the new
> cluster itself? That would be less confusing for users, and avoid some
> weirdness (like, if you drop the database "postgres" on the original, why
> does it reappear after an upgrade?).
I've wondered about that as well. There are some initdb-time options you can
set, but pg_upgrade could forward those.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Move defaults toward ICU in 16?
2023-02-17 08:06 Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 17:01 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 17:05 ` Re: Move defaults toward ICU in 16? Andres Freund <[email protected]>
@ 2023-02-17 18:00 ` Jeff Davis <[email protected]>
2023-02-17 18:09 ` Re: Move defaults toward ICU in 16? Andres Freund <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Jeff Davis @ 2023-02-17 18:00 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
On Fri, 2023-02-17 at 09:05 -0800, Andres Freund wrote:
> > Thinking about this more, it's not clear to me if this would be in
> > scope for pg_upgrade or not.
>
> I don't think we should consider changing the default collation
> provider
> without making this more seamless, one way or another.
I guess I'm fine hacking pg_upgrade, but I think I'd like to make it
conditional on this specific case: only perform the fixup if the old
cluster is 15 or earlier and using libc and the newer cluster is 16 or
later and using icu.
There's already a check that the new cluster is empty, so I think it's
safe to hack the pg_database locale fields.
Regards,
Jeff Davis
>
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Move defaults toward ICU in 16?
2023-02-17 08:06 Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 17:01 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 17:05 ` Re: Move defaults toward ICU in 16? Andres Freund <[email protected]>
2023-02-17 18:00 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
@ 2023-02-17 18:09 ` Andres Freund <[email protected]>
2023-02-17 20:36 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Andres Freund @ 2023-02-17 18:09 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
Hi,
On 2023-02-17 10:00:41 -0800, Jeff Davis wrote:
> I guess I'm fine hacking pg_upgrade, but I think I'd like to make it
> conditional on this specific case: only perform the fixup if the old
> cluster is 15 or earlier and using libc and the newer cluster is 16 or
> later and using icu.
-1. That's just going to cause pain one major version upgrade further down the
line. Why would we want to incur that pain?
> There's already a check that the new cluster is empty, so I think it's
> safe to hack the pg_database locale fields.
I don't think we need to, we do issue the CREATE DATABASEs. So we just need to
make sure that includes the collation provider info, and the proper template
database, in pg_upgrade mode.
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Move defaults toward ICU in 16?
2023-02-17 08:06 Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 17:01 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 17:05 ` Re: Move defaults toward ICU in 16? Andres Freund <[email protected]>
2023-02-17 18:00 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 18:09 ` Re: Move defaults toward ICU in 16? Andres Freund <[email protected]>
@ 2023-02-17 20:36 ` Jeff Davis <[email protected]>
2023-02-17 20:50 ` Re: Move defaults toward ICU in 16? Andres Freund <[email protected]>
0 siblings, 1 reply; 11+ messages in thread
From: Jeff Davis @ 2023-02-17 20:36 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
On Fri, 2023-02-17 at 10:09 -0800, Andres Freund wrote:
> -1. That's just going to cause pain one major version upgrade further
> down the
> line. Why would we want to incur that pain?
OK, we can just always do the fixup as long as the old one is libc and
the new one is ICU. I'm just trying to avoid this becoming a general
mechanism to fix up an incompatible new cluster.
> > There's already a check that the new cluster is empty, so I think
> > it's
> > safe to hack the pg_database locale fields.
>
> I don't think we need to, we do issue the CREATE DATABASEs. So we
> just need to
> make sure that includes the collation provider info, and the proper
> template
> database, in pg_upgrade mode.
We must fixup template1/postgres in the new cluster (change it to libc
to match the old cluster), because any objects existing in those
databases in the old cluster may depend on the default collation. I
don't see how we can do that without updating pg_database, so I'm not
following your point.
(I think you're right that template0 is optional; but since we're
fixing up the other databases it would be less surprising if we also
fixed up template0.)
And if we do fixup template0/template1/postgres to match the old
cluster, then CREATE DATABASE will have no issue.
--
Jeff Davis
PostgreSQL Contributor Team - AWS
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Move defaults toward ICU in 16?
2023-02-17 08:06 Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 17:01 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 17:05 ` Re: Move defaults toward ICU in 16? Andres Freund <[email protected]>
2023-02-17 18:00 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 18:09 ` Re: Move defaults toward ICU in 16? Andres Freund <[email protected]>
2023-02-17 20:36 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
@ 2023-02-17 20:50 ` Andres Freund <[email protected]>
0 siblings, 0 replies; 11+ messages in thread
From: Andres Freund @ 2023-02-17 20:50 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; Bruce Momjian <[email protected]>; +Cc: Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
Hi,
On 2023-02-17 12:36:05 -0800, Jeff Davis wrote:
> > > There's already a check that the new cluster is empty, so I think
> > > it's
> > > safe to hack the pg_database locale fields.
> >
> > I don't think we need to, we do issue the CREATE DATABASEs. So we
> > just need to
> > make sure that includes the collation provider info, and the proper
> > template
> > database, in pg_upgrade mode.
>
> We must fixup template1/postgres in the new cluster (change it to libc
> to match the old cluster), because any objects existing in those
> databases in the old cluster may depend on the default collation. I
> don't see how we can do that without updating pg_database, so I'm not
> following your point.
I think we just drop/recreate template1 and postgres during pg_upgrade. Yep,
looks like it. See create_new_objects():
/*
* template1 database will already exist in the target installation,
* so tell pg_restore to drop and recreate it; otherwise we would fail
* to propagate its database-level properties.
*/
create_opts = "--clean --create";
and then:
/*
* postgres database will already exist in the target installation, so
* tell pg_restore to drop and recreate it; otherwise we would fail to
* propagate its database-level properties.
*/
if (strcmp(old_db->db_name, "postgres") == 0)
create_opts = "--clean --create";
else
create_opts = "--create";
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Move defaults toward ICU in 16?
2023-02-17 08:06 Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 17:01 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
@ 2023-02-17 18:41 ` Justin Pryzby <[email protected]>
2 siblings, 0 replies; 11+ messages in thread
From: Justin Pryzby @ 2023-02-17 18:41 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers
On Fri, Feb 17, 2023 at 09:01:54AM -0800, Jeff Davis wrote:
> On Fri, 2023-02-17 at 00:06 -0800, Jeff Davis wrote:
> > On Tue, 2023-02-14 at 09:59 -0800, Andres Freund wrote:
> > > I am saying that pg_upgrade should be able to deal with the
> > > difference. The
> > > details of how to implement that, don't matter that much.
> >
> > To clarify, you're saying that pg_upgrade should simply update
> > pg_database to set the new databases' collation fields equal to that
> > of
> > the old cluster?
>
> Thinking about this more, it's not clear to me if this would be in
> scope for pg_upgrade or not. If pg_upgrade is fixing up the new cluster
> rather than checking for compatibility, why doesn't it just take over
> and do the initdb for the new cluster itself? That would be less
> confusing for users, and avoid some weirdness (like, if you drop the
> database "postgres" on the original, why does it reappear after an
> upgrade?).
>
> Someone might want to do something interesting to the new cluster
> before the upgrade, but it's not clear from the docs what would be both
> useful and safe.
This came up before - I'm of the opinion that it's unsupported and/or
useless to try to do anything on the new cluster between initdb and
pg_upgrade.
https://www.postgresql.org/message-id/[email protected]
https://www.postgresql.org/message-id/[email protected]
--
Justin
^ permalink raw reply [nested|flat] 11+ messages in thread
* Re: Move defaults toward ICU in 16?
2023-02-17 08:06 Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 17:01 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
@ 2023-02-18 08:38 ` Robert Haas <[email protected]>
2 siblings, 0 replies; 11+ messages in thread
From: Robert Haas @ 2023-02-18 08:38 UTC (permalink / raw)
To: Jeff Davis <[email protected]>; +Cc: Andres Freund <[email protected]>; Tom Lane <[email protected]>; Thomas Munro <[email protected]>; pgsql-hackers
On Fri, Feb 17, 2023 at 10:32 PM Jeff Davis <[email protected]> wrote:
> Thinking about this more, it's not clear to me if this would be in
> scope for pg_upgrade or not. If pg_upgrade is fixing up the new cluster
> rather than checking for compatibility, why doesn't it just take over
> and do the initdb for the new cluster itself? That would be less
> confusing for users, and avoid some weirdness (like, if you drop the
> database "postgres" on the original, why does it reappear after an
> upgrade?).
>
> Someone might want to do something interesting to the new cluster
> before the upgrade, but it's not clear from the docs what would be both
> useful and safe.
I agree with all of this. I think it would be fantastic if pg_upgrade
did the initdb itself. It would be simple to make this optional
behavior, too: if the destination directory does not exist or is
empty, initdb into it, otherwise skip that. That might be too
automagical, so we could add add a --no-initdb option. If not
specified, the destination directory must either not exist or be
empty; else it must exist and look like a data directory.
I completely concur with the idea that doing something with the new
cluster before the upgrade is weird, and I don't think we should
encourage people to do it. Nevertheless, as the threads to which
Justin linked probably say, I'm not sure that it's a good idea to
completely slam the door shut on that option. If we did want to move
in that direction, though, having pg_upgrade do the initdb would be an
excellent first step. We could at some later time decide to remove the
--no-initdb option; or maybe we'll decide that it's good to keep it
for emergencies, which is my present bias. In any event, the resulting
system would be more usable and less error-prone than what we have
today.
--
Robert Haas
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 11+ messages in thread
end of thread, other threads:[~2023-02-18 08:38 UTC | newest]
Thread overview: 11+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-02-02 17:03 [PATCH v7 1/3] Change XLogCtl->LogwrtResult to use atomic ops Alvaro Herrera <[email protected]>
2021-02-02 17:03 [PATCH v6 1/2] Change XLogCtl->LogwrtResult to use atomic ops Alvaro Herrera <[email protected]>
2023-02-17 08:06 Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 17:01 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 17:05 ` Re: Move defaults toward ICU in 16? Andres Freund <[email protected]>
2023-02-17 18:00 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 18:09 ` Re: Move defaults toward ICU in 16? Andres Freund <[email protected]>
2023-02-17 20:36 ` Re: Move defaults toward ICU in 16? Jeff Davis <[email protected]>
2023-02-17 20:50 ` Re: Move defaults toward ICU in 16? Andres Freund <[email protected]>
2023-02-17 18:41 ` Re: Move defaults toward ICU in 16? Justin Pryzby <[email protected]>
2023-02-18 08:38 ` Re: Move defaults toward ICU in 16? Robert Haas <[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