public inbox for [email protected]
help / color / mirror / Atom feed[PATCH v1 2/2] WIP: buffer alloc specialized for relation extension
8+ messages / 4 participants
[nested] [flat]
* [PATCH v1 2/2] WIP: buffer alloc specialized for relation extension
@ 2020-12-31 12:20 Luc Vlaming <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Luc Vlaming @ 2020-12-31 12:20 UTC (permalink / raw)
---
src/backend/storage/buffer/bufmgr.c | 280 +++++++++++++++++++++++++++-
src/include/storage/buf_internals.h | 2 +-
2 files changed, 275 insertions(+), 7 deletions(-)
diff --git a/src/backend/storage/buffer/bufmgr.c b/src/backend/storage/buffer/bufmgr.c
index d1ce88fee5..14f0923bb5 100644
--- a/src/backend/storage/buffer/bufmgr.c
+++ b/src/backend/storage/buffer/bufmgr.c
@@ -474,6 +474,10 @@ static BufferDesc *BufferAlloc(SMgrRelation smgr,
BlockNumber blockNum,
BufferAccessStrategy strategy,
bool *foundPtr);
+static BufferDesc *BufferAllocExtend(SMgrRelation smgr,
+ char relpersistence, ForkNumber forkNum,
+ BlockNumber blockNum, BufferAccessStrategy strategy,
+ LWLock** lastPartitionLock);
static void FlushBuffer(BufferDesc *buf, SMgrRelation reln);
static void AtProcExit_Buffers(int code, Datum arg);
static void CheckForBufferLeaks(void);
@@ -996,7 +1000,7 @@ ReadBufferExtendBulk(Relation reln, struct BulkInsertStateData* bistate)
BlockNumber firstBlock,
blockNum;
Block bufBlock;
- bool found;
+ LWLock* lastPartitionLock = NULL;
/* Open it at the smgr level if not already done */
RelationOpenSmgr(reln);
@@ -1007,7 +1011,7 @@ ReadBufferExtendBulk(Relation reln, struct BulkInsertStateData* bistate)
LockRelationForExtension(reln, ExclusiveLock);
firstBlock = smgrnblocks(smgr, MAIN_FORKNUM);
- smgrextend_count(smgr, MAIN_FORKNUM, firstBlock, bistate->empty_buffer, false, BULK_INSERT_BATCH_SIZE);
+ smgrextend_count(smgr, MAIN_FORKNUM, firstBlock, bistate->empty_buffer, true, BULK_INSERT_BATCH_SIZE);
UnlockRelationForExtension(reln, ExclusiveLock);
for (int i=0; i<BULK_INSERT_BATCH_SIZE; ++i)
@@ -1017,12 +1021,10 @@ ReadBufferExtendBulk(Relation reln, struct BulkInsertStateData* bistate)
pgstat_count_buffer_read(reln);
/* Make sure we will have room to remember the buffer pin */
ResourceOwnerEnlargeBuffers(CurrentResourceOwner);
- bufHdr = BufferAlloc(smgr, relpersistence, MAIN_FORKNUM, blockNum,
- bistate->strategy, &found);
+ bufHdr = BufferAllocExtend(smgr, relpersistence, MAIN_FORKNUM, blockNum,
+ bistate->strategy, &lastPartitionLock);
pgBufferUsage.shared_blks_written++;
- Assert(!found);
-
bufBlock = BufHdrGetBlock(bufHdr);
/* new buffers are zero-filled */
@@ -1033,6 +1035,9 @@ ReadBufferExtendBulk(Relation reln, struct BulkInsertStateData* bistate)
bistate->local_buffers[i] = BufferDescriptorGetBuffer(bufHdr);
}
+ Assert(lastPartitionLock);
+ LWLockRelease(lastPartitionLock);
+
bistate->local_buffers_idx = 0;
VacuumPageMiss += BULK_INSERT_BATCH_SIZE;
@@ -1408,6 +1413,269 @@ BufferAlloc(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
return buf;
}
+static BufferDesc *
+BufferAllocExtend(SMgrRelation smgr, char relpersistence, ForkNumber forkNum,
+ BlockNumber blockNum, BufferAccessStrategy strategy,
+ LWLock** lastPartitionLock)
+{
+ BufferTag newTag; /* identity of requested block */
+ uint32 newHash; /* hash value for newTag */
+ LWLock *newPartitionLock; /* buffer partition lock for it */
+ BufferTag oldTag; /* previous identity of selected buffer */
+ uint32 oldHash; /* hash value for oldTag */
+ LWLock *oldPartitionLock; /* buffer partition lock for it */
+ uint32 oldFlags;
+ int buf_id;
+ BufferDesc *buf;
+ uint32 buf_state;
+
+ Assert(lastPartitionLock);
+
+ /* create a tag so we can lookup the buffer */
+ INIT_BUFFERTAG(newTag, smgr->smgr_rnode.node, forkNum, blockNum);
+
+ /* determine its hash code and partition lock ID */
+ newHash = BufTableHashCode(&newTag);
+ newPartitionLock = BufMappingPartitionLock(newHash);
+
+ /* Loop here in case we have to try another victim buffer */
+ for (;;)
+ {
+ /*
+ * Ensure, while the spinlock's not yet held, that there's a free
+ * refcount entry.
+ */
+ ReservePrivateRefCountEntry();
+
+ /*
+ * Select a victim buffer. The buffer is returned with its header
+ * spinlock still held!
+ */
+ buf = StrategyGetBuffer(strategy, &buf_state);
+
+ Assert(BUF_STATE_GET_REFCOUNT(buf_state) == 0);
+
+ /* Must copy buffer flags while we still hold the spinlock */
+ oldFlags = buf_state & BUF_FLAG_MASK;
+
+ /* Pin the buffer and then release the buffer spinlock */
+ PinBuffer_Locked(buf);
+
+ /*
+ * If the buffer was dirty, try to write it out. There is a race
+ * condition here, in that someone might dirty it after we released it
+ * above, or even while we are writing it out (since our share-lock
+ * won't prevent hint-bit updates). We will recheck the dirty bit
+ * after re-locking the buffer header.
+ */
+ if (oldFlags & BM_DIRTY)
+ {
+ /*
+ * We need a share-lock on the buffer contents to write it out
+ * (else we might write invalid data, eg because someone else is
+ * compacting the page contents while we write). We must use a
+ * conditional lock acquisition here to avoid deadlock. Even
+ * though the buffer was not pinned (and therefore surely not
+ * locked) when StrategyGetBuffer returned it, someone else could
+ * have pinned and exclusive-locked it by the time we get here. If
+ * we try to get the lock unconditionally, we'd block waiting for
+ * them; if they later block waiting for us, deadlock ensues.
+ * (This has been observed to happen when two backends are both
+ * trying to split btree index pages, and the second one just
+ * happens to be trying to split the page the first one got from
+ * StrategyGetBuffer.)
+ */
+ if (LWLockConditionalAcquire(BufferDescriptorGetContentLock(buf),
+ LW_SHARED))
+ {
+ /*
+ * If using a nondefault strategy, and writing the buffer
+ * would require a WAL flush, let the strategy decide whether
+ * to go ahead and write/reuse the buffer or to choose another
+ * victim. We need lock to inspect the page LSN, so this
+ * can't be done inside StrategyGetBuffer.
+ */
+ if (strategy != NULL)
+ {
+ XLogRecPtr lsn;
+
+ /* Read the LSN while holding buffer header lock */
+ buf_state = LockBufHdr(buf);
+ lsn = BufferGetLSN(buf);
+ UnlockBufHdr(buf, buf_state);
+
+ if (XLogNeedsFlush(lsn) &&
+ StrategyRejectBuffer(strategy, buf))
+ {
+ /* Drop lock/pin and loop around for another buffer */
+ LWLockRelease(BufferDescriptorGetContentLock(buf));
+ UnpinBuffer(buf, true);
+ continue;
+ }
+ }
+
+ /* OK, do the I/O */
+ TRACE_POSTGRESQL_BUFFER_WRITE_DIRTY_START(forkNum, blockNum,
+ smgr->smgr_rnode.node.spcNode,
+ smgr->smgr_rnode.node.dbNode,
+ smgr->smgr_rnode.node.relNode);
+
+ FlushBuffer(buf, NULL);
+ LWLockRelease(BufferDescriptorGetContentLock(buf));
+
+ ScheduleBufferTagForWriteback(&BackendWritebackContext,
+ &buf->tag);
+
+ TRACE_POSTGRESQL_BUFFER_WRITE_DIRTY_DONE(forkNum, blockNum,
+ smgr->smgr_rnode.node.spcNode,
+ smgr->smgr_rnode.node.dbNode,
+ smgr->smgr_rnode.node.relNode);
+ }
+ else
+ {
+ /*
+ * Someone else has locked the buffer, so give it up and loop
+ * back to get another one.
+ */
+ UnpinBuffer(buf, true);
+ continue;
+ }
+ }
+
+ /*
+ * To change the association of a valid buffer, we'll need to have
+ * exclusive lock on both the old and new mapping partitions.
+ */
+ if (oldFlags & BM_TAG_VALID)
+ {
+ /*
+ * Need to compute the old tag's hashcode and partition lock ID.
+ * XXX is it worth storing the hashcode in BufferDesc so we need
+ * not recompute it here? Probably not.
+ */
+ oldTag = buf->tag;
+ oldHash = BufTableHashCode(&oldTag);
+ oldPartitionLock = BufMappingPartitionLock(oldHash);
+
+ if (*lastPartitionLock &&
+ (*lastPartitionLock != oldPartitionLock ||
+ *lastPartitionLock != newPartitionLock))
+ {
+ LWLockRelease(*lastPartitionLock);
+ *lastPartitionLock = NULL;
+ }
+
+ /*
+ * Must lock the lower-numbered partition first to avoid
+ * deadlocks.
+ */
+ if (oldPartitionLock < newPartitionLock)
+ {
+ Assert(*lastPartitionLock == NULL);
+ LWLockAcquire(oldPartitionLock, LW_EXCLUSIVE);
+ LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
+ }
+ else if (oldPartitionLock > newPartitionLock)
+ {
+ Assert(*lastPartitionLock == NULL);
+ LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
+ LWLockAcquire(oldPartitionLock, LW_EXCLUSIVE);
+ }
+ else
+ {
+ /* only one partition, only one lock */
+ if (*lastPartitionLock != newPartitionLock)
+ LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
+ }
+ }
+ else
+ {
+ /* if it wasn't valid, we need only the new partition */
+ if (*lastPartitionLock != newPartitionLock)
+ {
+ if (*lastPartitionLock)
+ LWLockRelease(*lastPartitionLock);
+ LWLockAcquire(newPartitionLock, LW_EXCLUSIVE);
+ }
+
+ /* remember we have no old-partition lock or tag */
+ oldPartitionLock = NULL;
+ /* keep the compiler quiet about uninitialized variables */
+ oldHash = 0;
+ }
+
+ *lastPartitionLock = newPartitionLock;
+
+ /*
+ * Try to make a hashtable entry for the buffer under its new tag.
+ * This could fail because while we were writing someone else
+ * allocated another buffer for the same block we want to read in.
+ * Note that we have not yet removed the hashtable entry for the old
+ * tag.
+ */
+ buf_id = BufTableInsert(&newTag, newHash, buf->buf_id);
+ Assert(buf_id < 0);
+
+ /*
+ * Need to lock the buffer header too in order to change its tag.
+ */
+ buf_state = LockBufHdr(buf);
+
+ /*
+ * Somebody could have pinned or re-dirtied the buffer while we were
+ * doing the I/O and making the new hashtable entry. If so, we can't
+ * recycle this buffer; we must undo everything we've done and start
+ * over with a new victim buffer.
+ */
+ oldFlags = buf_state & BUF_FLAG_MASK;
+ if (BUF_STATE_GET_REFCOUNT(buf_state) == 1 && !(oldFlags & BM_DIRTY))
+ break;
+
+ pg_unreachable();
+ }
+
+ /*
+ * Okay, it's finally safe to rename the buffer.
+ *
+ * Clearing BM_VALID here is necessary, clearing the dirtybits is just
+ * paranoia. We also reset the usage_count since any recency of use of
+ * the old content is no longer relevant. (The usage_count starts out at
+ * 1 so that the buffer can survive one clock-sweep pass.)
+ *
+ * Make sure BM_PERMANENT is set for buffers that must be written at every
+ * checkpoint. Unlogged buffers only need to be written at shutdown
+ * checkpoints, except for their "init" forks, which need to be treated
+ * just like permanent relations.
+ */
+ buf->tag = newTag;
+ buf_state &= ~(BM_VALID | BM_DIRTY | BM_JUST_DIRTIED |
+ BM_CHECKPOINT_NEEDED | BM_IO_ERROR | BM_PERMANENT |
+ BUF_USAGECOUNT_MASK);
+ if (relpersistence == RELPERSISTENCE_PERMANENT || forkNum == INIT_FORKNUM)
+ buf_state |= BM_TAG_VALID | BM_PERMANENT | BUF_USAGECOUNT_ONE;
+ else
+ buf_state |= BM_TAG_VALID | BUF_USAGECOUNT_ONE;
+
+ UnlockBufHdr(buf, buf_state);
+
+ if (oldPartitionLock != NULL)
+ {
+ BufTableDelete(&oldTag, oldHash);
+ if (oldPartitionLock != newPartitionLock)
+ LWLockRelease(oldPartitionLock);
+ }
+
+ /*
+ * Buffer contents are currently invalid. Try to get the io_in_progress
+ * lock. If StartBufferIO returns false, then someone else managed to
+ * read it before we did, so there's nothing left for BufferAlloc() to do.
+ */
+ StartBufferIO(buf, true);
+
+ return buf;
+}
+
+
/*
* InvalidateBuffer -- mark a shared buffer invalid and return it to the
* freelist.
diff --git a/src/include/storage/buf_internals.h b/src/include/storage/buf_internals.h
index 3377fa5676..277705f18c 100644
--- a/src/include/storage/buf_internals.h
+++ b/src/include/storage/buf_internals.h
@@ -124,7 +124,7 @@ typedef struct buftag
* NB: NUM_BUFFER_PARTITIONS must be a power of 2!
*/
#define BufTableHashPartition(hashcode) \
- ((hashcode) % NUM_BUFFER_PARTITIONS)
+ ((hashcode >> 7) % NUM_BUFFER_PARTITIONS)
#define BufMappingPartitionLock(hashcode) \
(&MainLWLockArray[BUFFER_MAPPING_LWLOCK_OFFSET + \
BufTableHashPartition(hashcode)].lock)
--
2.25.1
--------------065E078E0383D7C686F94091--
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: File locks for data directory lockfile in the context of Linux namespaces
@ 2026-01-17 15:26 Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Dolgov @ 2026-01-17 15:26 UTC (permalink / raw)
To: pgsql-hackers
> On Fri, Dec 19, 2025 at 03:27:40PM +0100, Dmitry Dolgov wrote:
> Hi,
>
> TL;DR This is a proposal to use file locking with a data directory lockfile at
> startup, which helps to avoid potential Linux PID namespace visibility issues.
Rebased and fixed a silly error with the flag availability. Feedback is
still welcome.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: File locks for data directory lockfile in the context of Linux namespaces
@ 2026-06-05 12:37 Ilmar Yunusov <[email protected]>
parent: Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Ilmar Yunusov @ 2026-06-05 12:37 UTC (permalink / raw)
To: [email protected]; +Cc: Dmitry Dolgov <[email protected]>
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
Hi,
I looked at v2, focusing on apply/build status and the PID namespace scenario
described in the cover letter.
I used the v2 patch from Dmitry's 2026-01-17 message, on origin/master at
4cb2a9863d89b320f37eb1bd76822f6f65e59311.
The patch applies cleanly with git am, and git diff --check reports no issues.
I built locally with:
./configure --prefix="$PWD/pg-install" --without-readline --without-zlib --without-icu
make -s -j8
make -s install
The build completed successfully. Configure found F_OFD_SETLK on this macOS
host too.
I also built and tested on Linux with the same configure options:
make -s -j3
make -s install
There, configure also found F_OFD_SETLK, and:
make -C src/test/regress check
passed; all regression tests passed.
For the namespace behavior, I used two PostgreSQL builds on the same Linux host:
unpatched master and v2. The test starts the first postmaster in one
pid/ipc/net namespace, then tries to start a second postmaster in another
pid/ipc/net namespace on the same data directory.
On unpatched master, both postmasters started. Both saw themselves as PID 2 in
their own PID namespace, and the second postmaster rewrote postmaster.pid. After
the second startup, the lock file contained the second socket directory:
2
.../pidns-base/data
1780661958
65437
.../pidns-base/sock2
With v2, the first postmaster started, and the second startup failed with:
FATAL: cannot lock the lock file "postmaster.pid"
HINT: Another server is starting.
So the patch fixes the concrete Linux PID namespace failure mode I tested.
One behavior I wanted to ask about: v2 also OFD-locks Unix socket lock files,
not only the data directory lockfile. That follows from calling
FlockDataDirLockFile() from the generic CreateLockFile() path, and I also saw
it at runtime. With one v2 postmaster using a Unix socket directory, /proc/locks
showed OFD write locks for both files:
.../data/postmaster.pid
.../sock/.s.PGSQL.65436.lock
The corresponding postmaster fds were:
/proc/835058/fd/5 -> .../data/postmaster.pid
/proc/835058/fd/8 -> .../sock/.s.PGSQL.65436.lock
I had expected the new lock to apply only to postmaster.pid, because the patch
title, commit message, helper name, and comments all describe the data directory
lockfile. The socket lockfile behavior therefore looked like either an
intentional scope expansion that should be named as such, or an accidental side
effect of using the generic CreateLockFile() path.
Is locking the socket lock file intentional here? If so, maybe the helper name,
comment, and fd tracking should reflect that broader scope. If not, perhaps the
new lock should be applied only for the isDDLock case; otherwise v2 changes
socket lockfile semantics too, not only postmaster.pid.
While reading that code, I also noticed a small error-path issue: DataDirLockFD
starts as 0, but if fcntl(F_OFD_SETLK) fails with a non-EAGAIN error the code
only emits a warning and does not assign a duplicate fd. UnlinkLockFiles()
then closes DataDirLockFD unconditionally. Initializing it to -1, closing it
conditionally, and checking dup(fd) would make that path more explicit.
Aside from those questions, this looks like a useful best-effort improvement to
me, and the namespace failure mode appears to be addressed by the OFD lock in
the Linux test above.
I did not test NFS behavior, older stable branches, Windows behavior, or
installcheck-world. Unprivileged namespace creation was not permitted on the
Linux host I used, so the namespace repro was run with sudo.
Regards,
Ilmar Yunusov
The new status of this patch is: Waiting on Author
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: File locks for data directory lockfile in the context of Linux namespaces
@ 2026-06-19 15:11 Dmitry Dolgov <[email protected]>
parent: Ilmar Yunusov <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Dolgov @ 2026-06-19 15:11 UTC (permalink / raw)
To: Ilmar Yunusov <[email protected]>; +Cc: [email protected]
> On Fri, Jun 05, 2026 at 12:37:26PM +0000, Ilmar Yunusov wrote:
> The following review has been posted through the commitfest application:
> make installcheck-world: not tested
> Implements feature: tested, passed
> Spec compliant: not tested
> Documentation: not tested
>
> Hi,
>
> I looked at v2, focusing on apply/build status and the PID namespace scenario
> described in the cover letter.
Thanks for the review and testing!
> Is locking the socket lock file intentional here? If so, maybe the helper name,
> comment, and fd tracking should reflect that broader scope. If not, perhaps the
> new lock should be applied only for the isDDLock case; otherwise v2 changes
> socket lockfile semantics too, not only postmaster.pid.
It's been a while, quite frankly I don't remember. On the face of it, I
think both the directory and socket lock files should be locked in the
same way, since both are equally susceptible for the failure scenario
described in this thread, even if the consequences of a failure are
different. Let me do some renaming to clarify that.
> While reading that code, I also noticed a small error-path issue: DataDirLockFD
> starts as 0, but if fcntl(F_OFD_SETLK) fails with a non-EAGAIN error the code
> only emits a warning and does not assign a duplicate fd. UnlinkLockFiles()
> then closes DataDirLockFD unconditionally. Initializing it to -1, closing it
> conditionally, and checking dup(fd) would make that path more explicit.
Good point, I'll address this in the next version.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: File locks for data directory lockfile in the context of Linux namespaces
@ 2026-06-23 14:29 Dmitry Dolgov <[email protected]>
parent: Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Dolgov @ 2026-06-23 14:29 UTC (permalink / raw)
To: Ilmar Yunusov <[email protected]>; +Cc: [email protected]
> On Fri, Jun 19, 2026 at 05:11:11PM +0200, Dmitry Dolgov wrote:
> > Is locking the socket lock file intentional here? If so, maybe the helper name,
> > comment, and fd tracking should reflect that broader scope. If not, perhaps the
> > new lock should be applied only for the isDDLock case; otherwise v2 changes
> > socket lockfile semantics too, not only postmaster.pid.
>
> It's been a while, quite frankly I don't remember. On the face of it, I
> think both the directory and socket lock files should be locked in the
> same way, since both are equally susceptible for the failure scenario
> described in this thread, even if the consequences of a failure are
> different. Let me do some renaming to clarify that.
>
> > While reading that code, I also noticed a small error-path issue: DataDirLockFD
> > starts as 0, but if fcntl(F_OFD_SETLK) fails with a non-EAGAIN error the code
> > only emits a warning and does not assign a duplicate fd. UnlinkLockFiles()
> > then closes DataDirLockFD unconditionally. Initializing it to -1, closing it
> > conditionally, and checking dup(fd) would make that path more explicit.
>
> Good point, I'll address this in the next version.
Something like this should be sufficient I think.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: File locks for data directory lockfile in the context of Linux namespaces
@ 2026-07-06 07:30 Ilmar Yunusov <[email protected]>
parent: Dmitry Dolgov <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Ilmar Yunusov @ 2026-07-06 07:30 UTC (permalink / raw)
To: [email protected]; +Cc: Dmitry Dolgov <[email protected]>
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: tested, passed
Spec compliant: not tested
Documentation: not tested
Hi,
I looked at v2, focusing on apply/build status and the PID namespace scenario
described in the cover letter.
I used the v2 patch from Dmitry's 2026-01-17 message, on origin/master at
4cb2a9863d89b320f37eb1bd76822f6f65e59311.
The patch applies cleanly with git am, and git diff --check reports no issues.
I built locally with:
./configure --prefix="$PWD/pg-install" --without-readline --without-zlib --without-icu
make -s -j8
make -s install
The build completed successfully. Configure found F_OFD_SETLK on this macOS
host too.
I also built and tested on Linux with the same configure options:
make -s -j3
make -s install
There, configure also found F_OFD_SETLK, and:
make -C src/test/regress check
passed; all regression tests passed.
For the namespace behavior, I used two PostgreSQL builds on the same Linux host:
unpatched master and v2. The test starts the first postmaster in one
pid/ipc/net namespace, then tries to start a second postmaster in another
pid/ipc/net namespace on the same data directory.
On unpatched master, both postmasters started. Both saw themselves as PID 2 in
their own PID namespace, and the second postmaster rewrote postmaster.pid. After
the second startup, the lock file contained the second socket directory:
2
.../pidns-base/data
1780661958
65437
.../pidns-base/sock2
With v2, the first postmaster started, and the second startup failed with:
FATAL: cannot lock the lock file "postmaster.pid"
HINT: Another server is starting.
So the patch fixes the concrete Linux PID namespace failure mode I tested.
One behavior I wanted to ask about: v2 also OFD-locks Unix socket lock files,
not only the data directory lockfile. That follows from calling
FlockDataDirLockFile() from the generic CreateLockFile() path, and I also saw
it at runtime. With one v2 postmaster using a Unix socket directory, /proc/locks
showed OFD write locks for both files:
.../data/postmaster.pid
.../sock/.s.PGSQL.65436.lock
The corresponding postmaster fds were:
/proc/835058/fd/5 -> .../data/postmaster.pid
/proc/835058/fd/8 -> .../sock/.s.PGSQL.65436.lock
I had expected the new lock to apply only to postmaster.pid, because the patch
title, commit message, helper name, and comments all describe the data directory
lockfile. The socket lockfile behavior therefore looked like either an
intentional scope expansion that should be named as such, or an accidental side
effect of using the generic CreateLockFile() path.
Is locking the socket lock file intentional here? If so, maybe the helper name,
comment, and fd tracking should reflect that broader scope. If not, perhaps the
new lock should be applied only for the isDDLock case; otherwise v2 changes
socket lockfile semantics too, not only postmaster.pid.
While reading that code, I also noticed a small error-path issue: DataDirLockFD
starts as 0, but if fcntl(F_OFD_SETLK) fails with a non-EAGAIN error the code
only emits a warning and does not assign a duplicate fd. UnlinkLockFiles()
then closes DataDirLockFD unconditionally. Initializing it to -1, closing it
conditionally, and checking dup(fd) would make that path more explicit.
Aside from those questions, this looks like a useful best-effort improvement to
me, and the namespace failure mode appears to be addressed by the OFD lock in
the Linux test above.
I did not test NFS behavior, older stable branches, Windows behavior, or
installcheck-world. Unprivileged namespace creation was not permitted on the
Linux host I used, so the namespace repro was run with sudo.
Regards,
Ilmar Yunusov
## v3 follow-up draft
Suggested CommitFest form:
- In response to: Dmitry's 2026-06-23 v3 message
- make installcheck-world: not tested
- Implements feature: tested, passed
- Spec compliant: not tested
- Documentation: not tested
- New status: Waiting on Author
- Attachments: none
Subject:
Re: File locks for data directory lockfile in the context of Linux namespaces
Body:
Hi,
I looked at v3, focusing on whether it addresses the two points from my
previous review, the Linux runtime behavior, and the current CFBot failures.
I used the v3 patch from Dmitry's 2026-06-23 message, on origin/master at
9d1188f29865e66c4196578501e74e8c815fba8d.
The patch applies cleanly with git am, and git diff --check reports no issues.
On Ubuntu 24.04.4, configure found F_OFD_SETLK, and this passed:
./configure --prefix=/home/master/pgcf-6335-v3-20260706/pg-install --without-readline --without-zlib --without-icu
make -s -j3
make -s install
make -C src/test/regress check
All 245 regression tests passed.
I re-ran the PID namespace reproducer from the earlier review against v3. The
first postmaster started in one pid/ipc/net namespace on the test data
directory. A second postmaster in another pid/ipc/net namespace on the same
data directory failed with:
FATAL: cannot lock the lock file "postmaster.pid"
HINT: Another server is starting.
So the concrete Linux PID namespace failure mode I tested still looks addressed
in v3.
I also checked the lockfile scope at runtime. With one v3 postmaster using a
Unix socket directory, /proc/<pid>/fd showed fds for both files:
.../data/postmaster.pid
.../sock/.s.PGSQL.65442.lock
/proc/locks also showed OFDLCK ADVISORY WRITE entries for both file inodes.
That matches the v3 scope where socket lockfiles are intentionally locked too.
The other question from my previous review also looks addressed: the single
static DataDirLockFD issue is replaced by per-lockfile fd tracking in the
lock_files list, with conditional close in UnlinkLockFiles().
There is still a build blocker, matching the current CFBot failures. v3 adds a
typedef named LockFile in src/backend/utils/init/miscinit.c, but that name
conflicts with the Windows LockFile() function. The CompilerWarnings log
reports, for example:
miscinit.c:79:3: error: 'LockFile' redeclared as different kind of symbol
and the Windows logs show the same issue:
miscinit.c(79): error C2365: 'LockFile': redefinition; previous definition was 'function'
There is also a missing semicolon in the non-F_OFD_SETLK branch:
miscinit.c:1169:18: error: expected ';' before '}' token
As a quick local compile check, in the temporary worktree only, renaming the
new typedef to LockFileData and adding the missing semicolon made the normal
macOS build pass. With HAVE_DECL_F_OFD_SETLK temporarily forced to 0 in the
generated pg_config.h, the affected miscinit.o target also compiled after those
two mechanical changes. I did not run a local Windows build.
I did not run installcheck-world, and did not test NFS behavior or
stable-branch backpatch applicability.
Regards,
Ilmar Yunusov
The new status of this patch is: Waiting on Author
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: File locks for data directory lockfile in the context of Linux namespaces
@ 2026-07-06 12:10 Dmitry Dolgov <[email protected]>
parent: Ilmar Yunusov <[email protected]>
0 siblings, 1 reply; 8+ messages in thread
From: Dmitry Dolgov @ 2026-07-06 12:10 UTC (permalink / raw)
To: Ilmar Yunusov <[email protected]>; +Cc: [email protected]
> On Mon, Jul 06, 2026 at 07:30:04AM +0000, Ilmar Yunusov wrote:
>
> There is still a build blocker, matching the current CFBot failures. v3 adds a
> typedef named LockFile in src/backend/utils/init/miscinit.c, but that name
> conflicts with the Windows LockFile() function. The CompilerWarnings log
> reports, for example:
Thanks for looking into the build failure. I wanted to check it out what
was happening on Windows, but after the migration from Cirrus to Github
one have to be logged in to see the build logs, and at that moment I
found myself logged off.
Regarding the name, I'm afraid LockFileData has a chance of causing some
confusion due to a common pattern, where structures are named with
"Data" suffix and a pointer type definition without. Since it's about a
name clash with an external library, I suggest PGLockFile instead, but
open for better suggestions.
^ permalink raw reply [nested|flat] 8+ messages in thread
* Re: File locks for data directory lockfile in the context of Linux namespaces
@ 2026-07-06 22:07 Zsolt Parragi <[email protected]>
parent: Dmitry Dolgov <[email protected]>
0 siblings, 0 replies; 8+ messages in thread
From: Zsolt Parragi @ 2026-07-06 22:07 UTC (permalink / raw)
To: [email protected]
Hello!
+ if (errno == EAGAIN)
+ ereport(FATAL,
+ (errcode(ERRCODE_LOCK_FILE_EXISTS),
According to fcntl.2, this should handle both EACCESS and EAGAIN:
ERRORS
EACCES or EAGAIN
Operation is prohibited by locks held by other processes.
+static int
+OFDLockFile(int fd, const char *filename)
+...
+ else
+ return dup(fd);
Isn't this missing an FD_CLOEXEC, so that launched processes doesn't
inherit it and keep the lock open possibly longer than needed?
Also, shouldn't the code verify the result of dup? (!= -1 / errno)
+ flock_fd = OFDLockFile(fd, filename);
Can't we leak flock_fd in the stale path?
+ * Close the file descriptor, which keeps the open file description
+ * lock.
+ */
+ if (lock_file->fd > 0)
+ close(lock_file->fd);
Shouldn't this check for >= 0?
+ elog(WARNING, "Failed locking file \"%s\", %m", filename);
This probably should be:
ereport(WARNING, (errcode_for_file_access(), errmsg("could not lock
file \"%s\": %m", filename)))
^ permalink raw reply [nested|flat] 8+ messages in thread
end of thread, other threads:[~2026-07-06 22:07 UTC | newest]
Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-12-31 12:20 [PATCH v1 2/2] WIP: buffer alloc specialized for relation extension Luc Vlaming <[email protected]>
2026-01-17 15:26 Re: File locks for data directory lockfile in the context of Linux namespaces Dmitry Dolgov <[email protected]>
2026-06-05 12:37 ` Re: File locks for data directory lockfile in the context of Linux namespaces Ilmar Yunusov <[email protected]>
2026-06-19 15:11 ` Re: File locks for data directory lockfile in the context of Linux namespaces Dmitry Dolgov <[email protected]>
2026-06-23 14:29 ` Re: File locks for data directory lockfile in the context of Linux namespaces Dmitry Dolgov <[email protected]>
2026-07-06 07:30 ` Re: File locks for data directory lockfile in the context of Linux namespaces Ilmar Yunusov <[email protected]>
2026-07-06 12:10 ` Re: File locks for data directory lockfile in the context of Linux namespaces Dmitry Dolgov <[email protected]>
2026-07-06 22:07 ` Re: File locks for data directory lockfile in the context of Linux namespaces Zsolt Parragi <[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