agora inbox for [email protected]
help / color / mirror / Atom feed[PATCH 2/2] Simplify XLogReader's open_segment API
3+ messages / 3 participants
[nested] [flat]
* [PATCH 2/2] Simplify XLogReader's open_segment API
@ 2020-05-08 21:03 Alvaro Herrera <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Alvaro Herrera @ 2020-05-08 21:03 UTC (permalink / raw)
Instead of returning the file descriptor, install it directly in
XLogReaderState->seg.ws_file.
---
src/backend/access/transam/xlogreader.c | 4 ++--
src/backend/access/transam/xlogutils.c | 32 ++++++++++++-------------
src/backend/replication/walsender.c | 12 ++++------
src/bin/pg_waldump/pg_waldump.c | 9 ++++---
src/include/access/xlogreader.h | 12 +++++-----
src/include/access/xlogutils.h | 2 +-
6 files changed, 33 insertions(+), 38 deletions(-)
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index f42dee2640..a533241370 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -1092,8 +1092,8 @@ WALRead(XLogReaderState *state,
state->routine.segment_close(state);
XLByteToSeg(recptr, nextSegNo, state->segcxt.ws_segsize);
- state->seg.ws_file = state->routine.segment_open(state, nextSegNo,
- &state->segcxt, &tli);
+ state->routine.segment_open(state, nextSegNo, &state->segcxt, &tli);
+ Assert(state->seg.ws_file >= 0); /* shouldn't happen */
/* Update the current segment info. */
state->seg.ws_tli = tli;
diff --git a/src/backend/access/transam/xlogutils.c b/src/backend/access/transam/xlogutils.c
index fc0bb7d059..1cc2c624a4 100644
--- a/src/backend/access/transam/xlogutils.c
+++ b/src/backend/access/transam/xlogutils.c
@@ -784,7 +784,7 @@ XLogReadDetermineTimeline(XLogReaderState *state, XLogRecPtr wantPage, uint32 wa
}
/* XLogReaderRoutine->segment_open callback for local pg_wal files */
-int
+void
wal_segment_open(XLogReaderState *state, XLogSegNo nextSegNo,
WALSegmentContext *segcxt, TimeLineID *tli_p)
{
@@ -793,22 +793,20 @@ wal_segment_open(XLogReaderState *state, XLogSegNo nextSegNo,
int fd;
XLogFilePath(path, tli, nextSegNo, segcxt->ws_segsize);
- fd = BasicOpenFile(path, O_RDONLY | PG_BINARY);
- if (fd >= 0)
- return fd;
-
- if (errno == ENOENT)
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("requested WAL segment %s has already been removed",
- path)));
- else
- ereport(ERROR,
- (errcode_for_file_access(),
- errmsg("could not open file \"%s\": %m",
- path)));
-
- return -1; /* keep compiler quiet */
+ state->seg.ws_file = BasicOpenFile(path, O_RDONLY | PG_BINARY);
+ if (state->seg.ws_file < 0)
+ {
+ if (errno == ENOENT)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("requested WAL segment %s has already been removed",
+ path)));
+ else
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not open file \"%s\": %m",
+ path)));
+ }
}
/* stock XLogReaderRoutine->segment_close callback */
diff --git a/src/backend/replication/walsender.c b/src/backend/replication/walsender.c
index ed8c08cb6a..b9f029d44f 100644
--- a/src/backend/replication/walsender.c
+++ b/src/backend/replication/walsender.c
@@ -248,7 +248,7 @@ static void LagTrackerWrite(XLogRecPtr lsn, TimestampTz local_flush_time);
static TimeOffset LagTrackerRead(int head, XLogRecPtr lsn, TimestampTz now);
static bool TransactionIdInRecentPast(TransactionId xid, uint32 epoch);
-static int WalSndSegmentOpen(XLogReaderState *state, XLogSegNo nextSegNo,
+static void WalSndSegmentOpen(XLogReaderState *state, XLogSegNo nextSegNo,
WALSegmentContext *segcxt, TimeLineID *tli_p);
static void UpdateSpillStats(LogicalDecodingContext *ctx);
@@ -2445,13 +2445,12 @@ WalSndKill(int code, Datum arg)
}
/* XLogReaderRoutine->segment_open callback */
-static int
+static void
WalSndSegmentOpen(XLogReaderState *state,
XLogSegNo nextSegNo, WALSegmentContext *segcxt,
TimeLineID *tli_p)
{
char path[MAXPGPATH];
- int fd;
/*-------
* When reading from a historic timeline, and there is a timeline switch
@@ -2488,9 +2487,9 @@ WalSndSegmentOpen(XLogReaderState *state,
}
XLogFilePath(path, *tli_p, nextSegNo, segcxt->ws_segsize);
- fd = BasicOpenFile(path, O_RDONLY | PG_BINARY);
- if (fd >= 0)
- return fd;
+ state->seg.ws_file = BasicOpenFile(path, O_RDONLY | PG_BINARY);
+ if (state->seg.ws_file >= 0)
+ return;
/*
* If the file is not found, assume it's because the standby asked for a
@@ -2513,7 +2512,6 @@ WalSndSegmentOpen(XLogReaderState *state,
(errcode_for_file_access(),
errmsg("could not open file \"%s\": %m",
path)));
- return -1; /* keep compiler quiet */
}
/*
diff --git a/src/bin/pg_waldump/pg_waldump.c b/src/bin/pg_waldump/pg_waldump.c
index 46734914b7..1a5c5a157c 100644
--- a/src/bin/pg_waldump/pg_waldump.c
+++ b/src/bin/pg_waldump/pg_waldump.c
@@ -280,7 +280,7 @@ identify_target_directory(char *directory, char *fname)
}
/* pg_waldump's XLogReaderRoutine->segment_open callback */
-static int
+static void
WALDumpOpenSegment(XLogReaderState *state,
XLogSegNo nextSegNo, WALSegmentContext *segcxt,
TimeLineID *tli_p)
@@ -300,9 +300,9 @@ WALDumpOpenSegment(XLogReaderState *state,
*/
for (tries = 0; tries < 10; tries++)
{
- fd = open_file_in_directory(segcxt->ws_dir, fname);
- if (fd >= 0)
- return fd;
+ state->seg.ws_file = open_file_in_directory(segcxt->ws_dir, fname);
+ if (state->seg.ws_file >= 0)
+ return;
if (errno == ENOENT)
{
int save_errno = errno;
@@ -318,7 +318,6 @@ WALDumpOpenSegment(XLogReaderState *state,
}
fatal_error("could not find file \"%s\": %m", fname);
- return -1; /* keep compiler quiet */
}
/*
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index e77f478d68..b73df02218 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -63,10 +63,10 @@ typedef int (*XLogPageReadCB) (XLogReaderState *xlogreader,
int reqLen,
XLogRecPtr targetRecPtr,
char *readBuf);
-typedef int (*WALSegmentOpenCB) (XLogReaderState *xlogreader,
- XLogSegNo nextSegNo,
- WALSegmentContext *segcxt,
- TimeLineID *tli_p);
+typedef void (*WALSegmentOpenCB) (XLogReaderState *xlogreader,
+ XLogSegNo nextSegNo,
+ WALSegmentContext *segcxt,
+ TimeLineID *tli_p);
typedef void (*WALSegmentCloseCB) (XLogReaderState *xlogreader);
typedef struct XLogReaderRoutine
@@ -94,8 +94,8 @@ typedef struct XLogReaderRoutine
XLogPageReadCB page_read;
/*
- * Callback to open the specified WAL segment for reading. The file
- * descriptor of the opened segment shall be returned. In case of
+ * Callback to open the specified WAL segment for reading. ->seg.ws_file
+ * shall be set to the file descriptor of the opened segment. In case of
* failure, an error shall be raised by the callback and it shall not
* return.
*
diff --git a/src/include/access/xlogutils.h b/src/include/access/xlogutils.h
index 68ce815476..b7bdc5db34 100644
--- a/src/include/access/xlogutils.h
+++ b/src/include/access/xlogutils.h
@@ -50,7 +50,7 @@ extern void FreeFakeRelcacheEntry(Relation fakerel);
extern int read_local_xlog_page(XLogReaderState *state,
XLogRecPtr targetPagePtr, int reqLen,
XLogRecPtr targetRecPtr, char *cur_page);
-extern int wal_segment_open(XLogReaderState *state,
+extern void wal_segment_open(XLogReaderState *state,
XLogSegNo nextSegNo,
WALSegmentContext *segcxt,
TimeLineID *tli_p);
--
2.20.1
--/04w6evG8XlLl3ft--
^ permalink raw reply [nested|flat] 3+ messages in thread
* Managing my own index partitions
@ 2022-08-08 18:40 Chris Cleveland <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Chris Cleveland @ 2022-08-08 18:40 UTC (permalink / raw)
To: pgsql-hackers
I'm building a Postgres index access method. For a variety of reasons it's
more efficient to store the index data in multiple physical files on disk
rather in the index's main fork.
I'm trying to create separate rels that can be created and destroyed by the
parent index access method. I've succeeded in creating them with
heap.c/heap_create() and also with heap_create_with_catalog(). Where I'm
struggling is in getting the rels to be dropped when the parent index is
dropped.
When I use heap_create(), followed by recordDependencyOn(parent_oid,
child_oid), the child doesn't get dropped when the parent index is dropped.
When I use heap_create_with_catalog(), followed by
recordDependencyOn(parent_oid, child_oid), I get a "cache lookup failed for
index <my_child_oid>" triggered from within CommandCounterIncrement(), and
no amount of spelunking in the code turns up which cache entry is the
problem.
Backing up just a minute, am I going about this the right way?
Should I be using heap_create() at all?
Is there some other way to do this?
***********************************************
Here is my code:
Oid namespaceId = get_namespace_oid("relevantdb", false);
Oid tablespaceId = parent_index_rel->rd_rel->reltablespace;
Oid new_seg_oid = get_new_filenode(tablespaceId);
char *new_seg_name = make_seg_name(parent_index_rel, new_seg_oid);
Oid new_seg_filenode = InvalidOid; // heap_create() will create it
// this is required, unfortunately. It goes in the relcache.
// it creates a totally empty tupdesc. The natts=1 arg is to avoid an
error.
TupleDesc segTupleDesc = CreateTemplateTupleDesc(1);
TupleDescInitEntry(segTupleDesc, (AttrNumber) 1,
"dummy",
INT4OID,
-1, 0);
bool shared_relation = parent_index_rel->rd_rel->relisshared;
bool mapped_relation = RelationIsMapped(parent_index_rel);
bool allow_system_table_mods = false;
// not used
TransactionId relfrozenxid;
MultiXactId relminmxid;
Relation seg_rel = heap_create(
new_seg_name,
namespaceId,
tablespaceId,
new_seg_oid,
new_seg_filenode,
0, // access method oid
segTupleDesc,
RELKIND_INDEX, // or RELKIND_RELATION or
RELKIND_PARTITIONED_INDEX?
RELPERSISTENCE,
shared_relation,
mapped_relation,
allow_system_table_mods,
&relfrozenxid,
&relminmxid
);
Assert(relfrozenxid == InvalidTransactionId);
Assert(relminmxid == InvalidMultiXactId);
Assert(new_seg_oid == RelationGetRelid(seg_rel));
// make changes visible
CommandCounterIncrement();
// record dependency so seg gets dropped when index dropped
Oid parent_oid = parent_index_rel->rd_id;
record_dependency(parent_oid, new_seg_oid);
table_close(seg_rel, NoLock); /* do not unlock till end of xact */
...
void record_dependency(Oid parent_oid, Oid child_oid) {
ObjectAddress baseobject;
ObjectAddress segobject;
baseobject.classId = IndexRelationId;
baseobject.objectId = parent_oid;
baseobject.objectSubId = 0;
segobject.classId = IndexRelationId;
segobject.objectId = child_oid;
segobject.objectSubId = 0;
recordDependencyOn(&segobject, &baseobject, DEPENDENCY_INTERNAL);
}
The code where I use heap_create_with_catalog() is substantially the same.
^ permalink raw reply [nested|flat] 3+ messages in thread
* [PATCH v5 3/3] Better express platform requirements in s_lock.h.
@ 2026-05-07 20:32 Nathan Bossart <[email protected]>
0 siblings, 0 replies; 3+ messages in thread
From: Nathan Bossart @ 2026-05-07 20:32 UTC (permalink / raw)
---
src/backend/storage/lmgr/s_lock.c | 2 ++
src/include/storage/s_lock.h | 59 +++++++++++++++++--------------
2 files changed, 35 insertions(+), 26 deletions(-)
diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c
index 6df568eccb3..34c6de66773 100644
--- a/src/backend/storage/lmgr/s_lock.c
+++ b/src/backend/storage/lmgr/s_lock.c
@@ -91,6 +91,7 @@ s_lock_stuck(const char *file, int line, const char *func)
#endif
}
+#ifdef USE_DEFAULT_S_LOCK
/*
* s_lock(lock) - platform-independent portion of waiting for a spinlock.
*/
@@ -110,6 +111,7 @@ s_lock(volatile slock_t *lock, const char *file, int line, const char *func)
return delayStatus.delays;
}
+#endif
#ifdef USE_DEFAULT_S_UNLOCK
void
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index fb872edd2f0..65f00c06518 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -44,23 +44,16 @@
* atomic test-and-set only when it appears free.
*
* TAS() and TAS_SPIN() are NOT part of the API, and should never be called
- * directly.
- *
- * CAUTION: on some platforms TAS() and/or TAS_SPIN() may sometimes report
- * failure to acquire a lock even when the lock is not locked. For example,
- * on Alpha TAS() will "fail" if interrupted. Therefore a retry loop must
- * always be used, even if you are certain the lock is free.
+ * directly. If a platform-specific TAS() is defined, the platform must
+ * _not_ define its own S_LOCK(). Conversely, if a platform-specific
+ * S_LOCK() is defined, the platform must _not_ define its own TAS().
+ * Currently, all supported platforms define TAS() and use the default
+ * S_LOCK() implementation, so that is probably a good place to start if
+ * adding a new one.
*
* It is the responsibility of these macros to make sure that the compiler
* does not re-order accesses to shared memory to precede the actual lock
- * acquisition, or follow the lock release. Prior to PostgreSQL 9.5, this
- * was the caller's responsibility, which meant that callers had to use
- * volatile-qualified pointers to refer to both the spinlock itself and the
- * shared data being accessed within the spinlocked critical section. This
- * was notationally awkward, easy to forget (and thus error-prone), and
- * prevented some useful compiler optimizations. For these reasons, we
- * now require that the macros themselves prevent compiler re-ordering,
- * so that the caller doesn't need to take special precautions.
+ * acquisition, or follow the lock release.
*
* On platforms with weak memory ordering, the TAS(), TAS_SPIN(), and
* S_UNLOCK() macros must further include hardware-level memory fence
@@ -72,7 +65,7 @@
*
* On most supported platforms, TAS() uses a tas() function written
* in assembly language to execute a hardware atomic-test-and-set
- * instruction. Equivalent OS-supplied mutex routines could be used too.
+ * instruction. Equivalent compiler intrinsics are another popular option.
*
*
* Portions Copyright (c) 1996-2026, PostgreSQL Global Development Group
@@ -642,20 +635,27 @@ spin_delay(void)
#endif /* !defined(TAS) */
-/* Blow up if we didn't have any way to do spinlocks */
-#ifndef TAS
-#error PostgreSQL does not have spinlock support on this platform. Please report this to [email protected].
-#endif
-
-
/*
* Default Definitions - override these above as needed.
*/
-#if !defined(S_LOCK)
+/*
+ * Make sure S_LOCK is defined, either explicitly for the platform or via a TAS
+ * macro for the platform. Exactly one of either S_LOCK or TAS should be
+ * defined for a supported platform at this point in the file.
+ */
+#if defined(S_LOCK)
+#if defined(TAS)
+#error Both TAS and S_LOCK defined on this platform. Please report this to [email protected].
+#endif
+#elif defined(TAS)
+#define USE_DEFAULT_S_LOCK
+extern int s_lock(volatile slock_t *lock, const char *file, int line, const char *func);
#define S_LOCK(lock) \
(TAS(lock) ? s_lock((lock), __FILE__, __LINE__, __func__) : 0)
-#endif /* S_LOCK */
+#else
+#error Neither TAS nor S_LOCK defined on this platform. Please report this to [email protected].
+#endif
#if !defined(S_UNLOCK)
/*
@@ -687,15 +687,22 @@ extern void s_unlock(volatile slock_t *lock);
#define SPIN_DELAY() ((void) 0)
#endif /* SPIN_DELAY */
-#if !defined(TAS_SPIN)
+/*
+ * We can only define TAS_SPIN if TAS was defined. Otherwise, the platform
+ * defined its own S_LOCK without TAS. Since TAS_SPIN is only used by the
+ * default S_LOCK's helper function, there's no need to define TAS_SPIN at all
+ * in that case, unless you plan to use it in a platform-specific S_LOCK helper
+ * function. (Note that we currently do not have any platforms that don't
+ * define TAS.)
+ */
+#if !defined(TAS_SPIN) && defined(TAS)
#define TAS_SPIN(lock) TAS(lock)
-#endif /* TAS_SPIN */
+#endif /* ! TAS_SPIN && TAS */
/*
* Platform-independent out-of-line support routines
*/
-extern int s_lock(volatile slock_t *lock, const char *file, int line, const char *func);
/* Support for dynamic adjustment of spins_per_delay */
#define DEFAULT_SPINS_PER_DELAY 100
--
2.50.1 (Apple Git-155)
--EzTP03ol5OjsRjZr--
^ permalink raw reply [nested|flat] 3+ messages in thread
end of thread, other threads:[~2026-05-07 20:32 UTC | newest]
Thread overview: 3+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-05-08 21:03 [PATCH 2/2] Simplify XLogReader's open_segment API Alvaro Herrera <[email protected]>
2022-08-08 18:40 Managing my own index partitions Chris Cleveland <[email protected]>
2026-05-07 20:32 [PATCH v5 3/3] Better express platform requirements in s_lock.h. Nathan Bossart <[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