Strange failure in LWLock on skink in REL9_5_STABLE
Hello,
Andres pinged me off-list to point out this failure after my commit fb389498be:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2018-09-20%2005%3A24%3A34
Change Set for this build:
fb389498be Tue Sep 18 11:19:22 2018 UTC Allow DSM allocation to be interrupted.
The failure looks like this:
! FATAL: semop(id=332464133) failed: Invalid argument
! CONTEXT: SQL statement "CREATE TEMP TABLE brin_result (cid tid)"
! PL/pgSQL function inline_code_block line 22 at SQL statement
! PANIC: queueing for lock while waiting on another one
! server closed the connection unexpectedly
! This probably means the server terminated abnormally
! before or while processing the request.
! connection to server was lost
I don't immediately see any connection between that particular commit,
which relates to the treatment of signals while allocating a DSM
segment, and the location of the first failure, which is in a
statement that is creating a temporary table. On the other hand skink
has been very stable lately. I'm also not sure how the FATAL error
and the PANIC are related (LWLockQueueSelf() has discovered that
MyProc->lwWaiting is already set). Though it's possible that the root
problem was something happening in any of the other parallel tests
running, I don't see how any of those (lock security_label tablesample
object_address rowsecurity collate spgist privileges matview
replica_identity brin gin gist groupingsets) would reach code touched
by that commit in 9.5, but I don't currently have any other ideas
about what happened here.
--
Thomas Munro
http://www.enterprisedb.com
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Andres pinged me off-list to point out this failure after my commit fb389498be:
! FATAL: semop(id=332464133) failed: Invalid argument
I was just looking at that, and my guess is that it was caused by
something doing an ipcrm or equivalent, and is unrelated to your patch.
Especially since skink has succeeded with that patch in several other
branches.
If it's repeatable, then it would be time to get excited.
regards, tom lane
On Fri, Sep 21, 2018 at 2:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Andres pinged me off-list to point out this failure after my commit fb389498be:
! FATAL: semop(id=332464133) failed: Invalid argument
I was just looking at that, and my guess is that it was caused by
something doing an ipcrm or equivalent, and is unrelated to your patch.
Especially since skink has succeeded with that patch in several other
branches.If it's repeatable, then it would be time to get excited.
I found this case from January:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2018-01-05%2000%3A10%3A03
! FATAL: semop(id=1313210374) failed: Invalid argument
! LINE 1: CREATE TABLE as_select1 AS SELECT * FROM pg_class WHERE relk...
EINVAL The semaphore set doesn't exist, or semid is less than zero,
or nsops has a nonpositive value.
--
Thomas Munro
http://www.enterprisedb.com
Thomas Munro <thomas.munro@enterprisedb.com> writes:
On Fri, Sep 21, 2018 at 2:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I was just looking at that, and my guess is that it was caused by
something doing an ipcrm or equivalent, and is unrelated to your patch.
Especially since skink has succeeded with that patch in several other
branches.
I found this case from January:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2018-01-05%2000%3A10%3A03
! FATAL: semop(id=1313210374) failed: Invalid argument
Uh-huh.
https://www.postgresql.org/docs/devel/static/kernel-resources.html#SYSTEMD-REMOVEIPC
regards, tom lane
On 2018-09-20 22:59:29 -0400, Tom Lane wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
Andres pinged me off-list to point out this failure after my commit fb389498be:
! FATAL: semop(id=332464133) failed: Invalid argument
I was just looking at that, and my guess is that it was caused by
something doing an ipcrm or equivalent, and is unrelated to your patch.
Especially since skink has succeeded with that patch in several other
branches.
I'm (hopefully) the only person with access to that machine, and I
certainly didn't do so. Nor are there script I know of that'd do
so. There's not been a lot of instability on skink, so it's certainly
quite weird.
I'm quite suspicious of the logic around:
/*
* If we received a query cancel or termination signal, we will have
* EINTR set here. If the caller said that errors are OK here, check
* for interrupts immediately.
*/
if (errno == EINTR && elevel >= ERROR)
CHECK_FOR_INTERRUPTS();
because it seems far from guaranteed to do anything meaningful as I
don't see a guarantee that interrupts are active at that point (e.g. it
seems quite reasonable to hold an lwlock while resizing).
Afaict that might cause problems at a later stage, because at that point
we've not adjusted the actual mapping, but *have* ftruncate()ed it. If
there's actual data in the mapping, that certainly could cause trouble.
In fact, while this commit has expanded the size of the problem, I fail
to see how the error handling for resizing is correct. It's fine to fail
in the ftruncate() itself - at that point no changes have been made -,
but I don't think it's currently ok for posix_fallocate() to ever error
out.
It's not clear to me how that'd be problematic in 9.5 of all releases
however.
If it's repeatable, then it would be time to get excited.
Yea, I guess we'll have to wait :/.
Greetings,
Andres Freund
On 2018-09-20 23:15:45 -0400, Tom Lane wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
On Fri, Sep 21, 2018 at 2:59 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
I was just looking at that, and my guess is that it was caused by
something doing an ipcrm or equivalent, and is unrelated to your patch.
Especially since skink has succeeded with that patch in several other
branches.I found this case from January:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=skink&dt=2018-01-05%2000%3A10%3A03
! FATAL: semop(id=1313210374) failed: Invalid argument
Uh-huh.
https://www.postgresql.org/docs/devel/static/kernel-resources.html#SYSTEMD-REMOVEIPC
That shouldn't be relevant here - I'm not running the buildfarm from an
interactive session and then logging out. So that code shouldn't
trigger. I've made sure that the setting is off now however, I'm not
trusting the related logic very much...
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
On 2018-09-20 23:15:45 -0400, Tom Lane wrote:
https://www.postgresql.org/docs/devel/static/kernel-resources.html#SYSTEMD-REMOVEIPC
That shouldn't be relevant here - I'm not running the buildfarm from an
interactive session and then logging out. So that code shouldn't
trigger.
Well, the reason that systemd behavior is so brain-dead is exactly that
it will trigger against processes that you didn't launch interactively.
IIUC, you just have to log in and out of that account, and anything that
was running in the background (eg cron jobs) under the same userID gets
clobbered.
I've made sure that the setting is off now however, I'm not
trusting the related logic very much...
Was it on before?
regards, tom lane
On Fri, Sep 21, 2018 at 3:21 PM Andres Freund <andres@anarazel.de> wrote:
I'm quite suspicious of the logic around:
/*
* If we received a query cancel or termination signal, we will have
* EINTR set here. If the caller said that errors are OK here, check
* for interrupts immediately.
*/
if (errno == EINTR && elevel >= ERROR)
CHECK_FOR_INTERRUPTS();because it seems far from guaranteed to do anything meaningful as I
don't see a guarantee that interrupts are active at that point (e.g. it
seems quite reasonable to hold an lwlock while resizing).
Right, in that case CFI does nothing and you get the following
ereport() instead, so the user sees an unsightly "Interrupt system
call" message (or however your strerror() spells it). That would
actually happen in the dsa.c case for example (something that should
be improved especially now that dsm_create() is so slow on Linux,
independently of all this). That's probably not a great user
experience, but I'm not sure if the fact that it "works around" the
suppression of interrupts while LWLocks are held by converting them
into errors is a more serious problem or not. The caller has to be
ready for errors to be raised here in any case.
Afaict that might cause problems at a later stage, because at that point
we've not adjusted the actual mapping, but *have* ftruncate()ed it. If
there's actual data in the mapping, that certainly could cause trouble.In fact, while this commit has expanded the size of the problem, I fail
to see how the error handling for resizing is correct. It's fine to fail
in the ftruncate() itself - at that point no changes have been made -,
but I don't think it's currently ok for posix_fallocate() to ever error
out.
Right, independently of this commit, dsm_resize() might have done the
actual truncation when the error is reported. That's not good if the
caller plans to do anything other than abandon ship. None of this
applies to dsm_create() though, which uses the same code path but
knows how to clean up. There may be ways to fix the dsm_resize() path
based on the observation that you don't need to fallocate() if you
made the mapping smaller, and if you made it bigger then you could
always undo that on error (or not) and you haven't thrown away any
data. Hmm... I note that there are actually no callers of
dsm_resize(), and it's not implemented on Windows or SystemV.
--
Thomas Munro
http://www.enterprisedb.com
Thomas Munro <thomas.munro@enterprisedb.com> writes:
... There may be ways to fix the dsm_resize() path
based on the observation that you don't need to fallocate() if you
made the mapping smaller, and if you made it bigger then you could
always undo that on error (or not) and you haven't thrown away any
data. Hmm... I note that there are actually no callers of
dsm_resize(), and it's not implemented on Windows or SystemV.
Why would we fix it rather than just removing it?
regards, tom lane
On 2018-09-20 23:46:54 -0400, Tom Lane wrote:
Andres Freund <andres@anarazel.de> writes:
I've made sure that the setting is off now however, I'm not
trusting the related logic very much...Was it on before?
Yes.
On Fri, Sep 21, 2018 at 4:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
... There may be ways to fix the dsm_resize() path
based on the observation that you don't need to fallocate() if you
made the mapping smaller, and if you made it bigger then you could
always undo that on error (or not) and you haven't thrown away any
data. Hmm... I note that there are actually no callers of
dsm_resize(), and it's not implemented on Windows or SystemV.
Erm, actually you probably only need to do ftruncate() *or*
posix_fallocate(), depending on the direction of the resize. Doing
both is redundant and introduces this theoretical hazard (in practice
I'd be surprised if fallocate() really can fail after you shrank a
file that was already fully allocated).
Why would we fix it rather than just removing it?
I assumed we wouldn't remove an extern C function extension code
somewhere might use. Though admittedly I'd be surprised if anyone
used this one.
--
Thomas Munro
http://www.enterprisedb.com
Thomas Munro <thomas.munro@enterprisedb.com> writes:
On Fri, Sep 21, 2018 at 4:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Why would we fix it rather than just removing it?
I assumed we wouldn't remove an extern C function extension code
somewhere might use. Though admittedly I'd be surprised if anyone
used this one.
Unless it looks practical to support this behavior in the Windows
and SysV cases, I think we should get rid of it rather than expend
effort on supporting it for just some platforms.
regards, tom lane
On Fri, Sep 21, 2018 at 4:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
On Fri, Sep 21, 2018 at 4:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Why would we fix it rather than just removing it?
I assumed we wouldn't remove an extern C function extension code
somewhere might use. Though admittedly I'd be surprised if anyone
used this one.Unless it looks practical to support this behavior in the Windows
and SysV cases, I think we should get rid of it rather than expend
effort on supporting it for just some platforms.
We can remove it in back-branches without breaking API compatibility:
1. Change dsm_impl_can_resize() to return false unconditionally (I
suppose client code is supposed to check this before using
dsm_resize(), though I'm not sure why it has an "impl" in its name if
it's part of the public interface of this module).
2. Change dsm_resize() and dsm_remap() to raise an error conditionally.
3. Rip out the DSM_OP_RESIZE cases from various places.
Then in master, remove all of those functions completely. However,
I'd feel like a bit of a vandal. Robert and Amit probably had plans
for that code...?
--
Thomas Munro
http://www.enterprisedb.com
On 2018-09-22 08:54:57 +1200, Thomas Munro wrote:
On Fri, Sep 21, 2018 at 4:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
On Fri, Sep 21, 2018 at 4:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Why would we fix it rather than just removing it?
I assumed we wouldn't remove an extern C function extension code
somewhere might use. Though admittedly I'd be surprised if anyone
used this one.Unless it looks practical to support this behavior in the Windows
and SysV cases, I think we should get rid of it rather than expend
effort on supporting it for just some platforms.We can remove it in back-branches without breaking API compatibility:
1. Change dsm_impl_can_resize() to return false unconditionally (I
suppose client code is supposed to check this before using
dsm_resize(), though I'm not sure why it has an "impl" in its name if
it's part of the public interface of this module).
2. Change dsm_resize() and dsm_remap() to raise an error conditionally.
3. Rip out the DSM_OP_RESIZE cases from various places.Then in master, remove all of those functions completely. However,
I'd feel like a bit of a vandal. Robert and Amit probably had plans
for that code...?
Robert, Amit: ^
Greetings,
Andres Freund
On Sat, Sep 22, 2018 at 2:28 AM Andres Freund <andres@anarazel.de> wrote:
On 2018-09-22 08:54:57 +1200, Thomas Munro wrote:
On Fri, Sep 21, 2018 at 4:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Thomas Munro <thomas.munro@enterprisedb.com> writes:
On Fri, Sep 21, 2018 at 4:06 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Why would we fix it rather than just removing it?
I assumed we wouldn't remove an extern C function extension code
somewhere might use. Though admittedly I'd be surprised if anyone
used this one.Unless it looks practical to support this behavior in the Windows
and SysV cases, I think we should get rid of it rather than expend
effort on supporting it for just some platforms.We can remove it in back-branches without breaking API compatibility:
1. Change dsm_impl_can_resize() to return false unconditionally (I
suppose client code is supposed to check this before using
dsm_resize(), though I'm not sure why it has an "impl" in its name if
it's part of the public interface of this module).
2. Change dsm_resize() and dsm_remap() to raise an error conditionally.
3. Rip out the DSM_OP_RESIZE cases from various places.Then in master, remove all of those functions completely. However,
I'd feel like a bit of a vandal. Robert and Amit probably had plans
for that code...?Robert, Amit: ^
I went through and check the original proposal [1]/messages/by-id/CA+TgmoaDqDUgt=4Zs_QPOnBt=EstEaVNP+5t+m=FPNWshiPR3A@mail.gmail.com to see if any use
case is mentioned there, but nothing related has been discussed. I
couldn't think of much use of this facility except maybe for something
like parallelizing correalated sub-queries where the size of outer var
can change across executions and we might need to resize the initially
allocated memory. This is just a wild thought, I don't have any
concrete idea about this. Having said that, I don't object to
removing this especially because the implementation doesn't seem to be
complete. In future, if someone needs such a facility, they can first
develop a complete version of this API.
[1]: /messages/by-id/CA+TgmoaDqDUgt=4Zs_QPOnBt=EstEaVNP+5t+m=FPNWshiPR3A@mail.gmail.com
--
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com
On Sat, Sep 22, 2018 at 4:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Sat, Sep 22, 2018 at 2:28 AM Andres Freund <andres@anarazel.de> wrote:
On 2018-09-22 08:54:57 +1200, Thomas Munro wrote:
On Fri, Sep 21, 2018 at 4:43 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Unless it looks practical to support this behavior in the Windows
and SysV cases, I think we should get rid of it rather than expend
effort on supporting it for just some platforms.We can remove it in back-branches without breaking API compatibility:
1. Change dsm_impl_can_resize() to return false unconditionally (I
suppose client code is supposed to check this before using
dsm_resize(), though I'm not sure why it has an "impl" in its name if
it's part of the public interface of this module).
2. Change dsm_resize() and dsm_remap() to raise an error conditionally.
3. Rip out the DSM_OP_RESIZE cases from various places.Then in master, remove all of those functions completely. However,
I'd feel like a bit of a vandal. Robert and Amit probably had plans
for that code...?Robert, Amit: ^
I went through and check the original proposal [1] to see if any use
case is mentioned there, but nothing related has been discussed. I
couldn't think of much use of this facility except maybe for something
like parallelizing correalated sub-queries where the size of outer var
can change across executions and we might need to resize the initially
allocated memory. This is just a wild thought, I don't have any
concrete idea about this. Having said that, I don't object to
removing this especially because the implementation doesn't seem to be
complete. In future, if someone needs such a facility, they can first
develop a complete version of this API.
Thanks for looking into that. Here's a pair of draft patches to
disable and then remove dsm_resize() and dsm_map().
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
0001-Desupport-dsm_resize-and-dsm_remap.patchapplication/octet-stream; name=0001-Desupport-dsm_resize-and-dsm_remap.patchDownload
From d0d30d13e8dacbfc837386db83cbd585b3885ccd Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@enterprisedb.com>
Date: Tue, 25 Sep 2018 15:39:28 +1200
Subject: [PATCH 1/2] Desupport dsm_resize() and dsm_remap().
dsm_resize() was not used, had a subtle bug on one platform, and
wasn't supported on all platforms. If there are any external users
of the interface, they should be calling dsm_impl_can_resize() and
falling back to different behavior when it's not available. Update
that function to return false always. Later we'll remove the
interface completely.
Back-patch to 9.4 where DSM first landed.
Author: Thomas Munro
Reported-by: Andres Freund
Reviewed-by:
Discussion: https://postgr.es/m/CAA4eK1%2B%3DyAFUvpFoHXFi_gm8YqmXN-TtkFH%2BVYjvDLS6-SFq-Q%40mail.gmail.com
---
src/backend/storage/ipc/dsm.c | 20 +---
src/backend/storage/ipc/dsm_impl.c | 145 ++++-------------------------
src/include/storage/dsm_impl.h | 1 -
3 files changed, 21 insertions(+), 145 deletions(-)
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index 9629f22f7a..af1162c6e8 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -655,34 +655,22 @@ dsm_detach_all(void)
/*
* Resize an existing shared memory segment.
- *
- * This may cause the shared memory segment to be remapped at a different
- * address. For the caller's convenience, we return the mapped address.
*/
void *
dsm_resize(dsm_segment *seg, Size size)
{
- Assert(seg->control_slot != INVALID_CONTROL_SLOT);
- dsm_impl_op(DSM_OP_RESIZE, seg->handle, size, &seg->impl_private,
- &seg->mapped_address, &seg->mapped_size, ERROR);
- return seg->mapped_address;
+ elog(ERROR, "dsm_resize not supported");
+ return NULL;
}
/*
* Remap an existing shared memory segment.
- *
- * This is intended to be used when some other process has extended the
- * mapping using dsm_resize(), but we've still only got the initial
- * portion mapped. Since this might change the address at which the
- * segment is mapped, we return the new mapped address.
*/
void *
dsm_remap(dsm_segment *seg)
{
- dsm_impl_op(DSM_OP_ATTACH, seg->handle, 0, &seg->impl_private,
- &seg->mapped_address, &seg->mapped_size, ERROR);
-
- return seg->mapped_address;
+ elog(ERROR, "dsm_remap not supported");
+ return NULL;
}
/*
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index 70f899e765..fe581afffb 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -127,14 +127,9 @@ int dynamic_shared_memory_type;
* map it.
*
* DSM_OP_ATTACH. Map the segment, whose size must be the request_size.
- * The segment may already be mapped; any existing mapping should be removed
- * before creating a new one.
*
* DSM_OP_DETACH. Unmap the segment.
*
- * DSM_OP_RESIZE. Resize the segment to the given request_size and
- * remap the segment at that new size.
- *
* DSM_OP_DESTROY. Unmap the segment, if it is mapped. Destroy the
* segment.
*
@@ -142,8 +137,7 @@ int dynamic_shared_memory_type;
* op: The operation to be performed.
* handle: The handle of an existing object, or for DSM_OP_CREATE, the
* a new handle the caller wants created.
- * request_size: For DSM_OP_CREATE, the requested size. For DSM_OP_RESIZE,
- * the new size. Otherwise, 0.
+ * request_size: For DSM_OP_CREATE, the requested size. Otherwise, 0.
* impl_private: Private, implementation-specific data. Will be a pointer
* to NULL for the first operation on a shared memory segment within this
* backend; thereafter, it will point to the value to which it was set
@@ -165,7 +159,7 @@ dsm_impl_op(dsm_op op, dsm_handle handle, Size request_size,
void **impl_private, void **mapped_address, Size *mapped_size,
int elevel)
{
- Assert(op == DSM_OP_CREATE || op == DSM_OP_RESIZE || request_size == 0);
+ Assert(op == DSM_OP_CREATE || request_size == 0);
Assert((op != DSM_OP_CREATE && op != DSM_OP_ATTACH) ||
(*mapped_address == NULL && *mapped_size == 0));
@@ -199,28 +193,16 @@ dsm_impl_op(dsm_op op, dsm_handle handle, Size request_size,
}
/*
- * Does the current dynamic shared memory implementation support resizing
- * segments? (The answer here could be platform-dependent in the future,
- * since AIX allows shmctl(shmid, SHM_RESIZE, &buffer), though you apparently
- * can't resize segments to anything larger than 256MB that way. For now,
- * we keep it simple.)
+ * Previously we supported resizing DSM segments on some platforms. Since
+ * the work was never extended to all platforms so that it couldn't be relied
+ * on in general, and potential bugs were discovered on one one platform
+ * where we did support it, we decided to remove this. The interface will
+ * disappear completely in a later release.
*/
bool
dsm_impl_can_resize(void)
{
- switch (dynamic_shared_memory_type)
- {
- case DSM_IMPL_POSIX:
- return true;
- case DSM_IMPL_SYSV:
- return false;
- case DSM_IMPL_WINDOWS:
- return false;
- case DSM_IMPL_MMAP:
- return true;
- default:
- return false; /* should not happen */
- }
+ return false;
}
#ifdef USE_DSM_POSIX
@@ -296,7 +278,7 @@ dsm_impl_posix(dsm_op op, dsm_handle handle, Size request_size,
/*
* If we're attaching the segment, determine the current size; if we are
- * creating or resizing the segment, set the size to the requested value.
+ * creating the segment, set the size to the requested value.
*/
if (op == DSM_OP_ATTACH)
{
@@ -319,16 +301,14 @@ dsm_impl_posix(dsm_op op, dsm_handle handle, Size request_size,
}
request_size = st.st_size;
}
- else if (*mapped_size != request_size &&
- dsm_impl_posix_resize(fd, request_size) != 0)
+ else if (dsm_impl_posix_resize(fd, request_size) != 0)
{
int save_errno;
/* Back out what's already been done. */
save_errno = errno;
close(fd);
- if (op == DSM_OP_CREATE)
- shm_unlink(name);
+ shm_unlink(name);
errno = save_errno;
/*
@@ -346,35 +326,6 @@ dsm_impl_posix(dsm_op op, dsm_handle handle, Size request_size,
return false;
}
- /*
- * If we're reattaching or resizing, we must remove any existing mapping,
- * unless we've already got the right thing mapped.
- */
- if (*mapped_address != NULL)
- {
- if (*mapped_size == request_size)
- return true;
- if (munmap(*mapped_address, *mapped_size) != 0)
- {
- int save_errno;
-
- /* Back out what's already been done. */
- save_errno = errno;
- close(fd);
- if (op == DSM_OP_CREATE)
- shm_unlink(name);
- errno = save_errno;
-
- ereport(elevel,
- (errcode_for_dynamic_shared_memory(),
- errmsg("could not unmap shared memory segment \"%s\": %m",
- name)));
- return false;
- }
- *mapped_address = NULL;
- *mapped_size = 0;
- }
-
/* Map it. */
address = mmap(NULL, request_size, PROT_READ | PROT_WRITE,
MAP_SHARED | MAP_HASSEMAPHORE | MAP_NOSYNC, fd, 0);
@@ -457,10 +408,9 @@ dsm_impl_posix_resize(int fd, off_t size)
* Operating system primitives to support System V shared memory.
*
* System V shared memory segments are manipulated using shmget(), shmat(),
- * shmdt(), and shmctl(). There's no portable way to resize such
- * segments. As the default allocation limits for System V shared memory
- * are usually quite low, the POSIX facilities may be preferable; but
- * those are not supported everywhere.
+ * shmdt(), and shmctl(). As the default allocation limits for System V
+ * shared memory are usually quite low, the POSIX facilities may be
+ * preferable; but those are not supported everywhere.
*/
static bool
dsm_impl_sysv(dsm_op op, dsm_handle handle, Size request_size,
@@ -473,13 +423,6 @@ dsm_impl_sysv(dsm_op op, dsm_handle handle, Size request_size,
char name[64];
int *ident_cache;
- /* Resize is not supported for System V shared memory. */
- if (op == DSM_OP_RESIZE)
- {
- elog(elevel, "System V shared memory segments cannot be resized");
- return false;
- }
-
/* Since resize isn't supported, reattach is a no-op. */
if (op == DSM_OP_ATTACH && *mapped_address != NULL)
return true;
@@ -670,13 +613,6 @@ dsm_impl_windows(dsm_op op, dsm_handle handle, Size request_size,
char name[64];
MEMORY_BASIC_INFORMATION info;
- /* Resize is not supported for Windows shared memory. */
- if (op == DSM_OP_RESIZE)
- {
- elog(elevel, "Windows shared memory segments cannot be resized");
- return false;
- }
-
/* Since resize isn't supported, reattach is a no-op. */
if (op == DSM_OP_ATTACH && *mapped_address != NULL)
return true;
@@ -905,7 +841,7 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
/*
* If we're attaching the segment, determine the current size; if we are
- * creating or resizing the segment, set the size to the requested value.
+ * creating the segment, set the size to the requested value.
*/
if (op == DSM_OP_ATTACH)
{
@@ -928,24 +864,7 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
}
request_size = st.st_size;
}
- else if (*mapped_size > request_size && ftruncate(fd, request_size))
- {
- int save_errno;
-
- /* Back out what's already been done. */
- save_errno = errno;
- CloseTransientFile(fd);
- if (op == DSM_OP_CREATE)
- unlink(name);
- errno = save_errno;
-
- ereport(elevel,
- (errcode_for_dynamic_shared_memory(),
- errmsg("could not resize shared memory segment \"%s\" to %zu bytes: %m",
- name, request_size)));
- return false;
- }
- else if (*mapped_size < request_size)
+ else
{
/*
* Allocate a buffer full of zeros.
@@ -985,8 +904,7 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
/* Back out what's already been done. */
save_errno = errno;
CloseTransientFile(fd);
- if (op == DSM_OP_CREATE)
- unlink(name);
+ unlink(name);
errno = save_errno ? save_errno : ENOSPC;
ereport(elevel,
@@ -997,35 +915,6 @@ dsm_impl_mmap(dsm_op op, dsm_handle handle, Size request_size,
}
}
- /*
- * If we're reattaching or resizing, we must remove any existing mapping,
- * unless we've already got the right thing mapped.
- */
- if (*mapped_address != NULL)
- {
- if (*mapped_size == request_size)
- return true;
- if (munmap(*mapped_address, *mapped_size) != 0)
- {
- int save_errno;
-
- /* Back out what's already been done. */
- save_errno = errno;
- CloseTransientFile(fd);
- if (op == DSM_OP_CREATE)
- unlink(name);
- errno = save_errno;
-
- ereport(elevel,
- (errcode_for_dynamic_shared_memory(),
- errmsg("could not unmap shared memory segment \"%s\": %m",
- name)));
- return false;
- }
- *mapped_address = NULL;
- *mapped_size = 0;
- }
-
/* Map it. */
address = mmap(NULL, request_size, PROT_READ | PROT_WRITE,
MAP_SHARED | MAP_HASSEMAPHORE | MAP_NOSYNC, fd, 0);
diff --git a/src/include/storage/dsm_impl.h b/src/include/storage/dsm_impl.h
index e7acdff355..c8adc425fd 100644
--- a/src/include/storage/dsm_impl.h
+++ b/src/include/storage/dsm_impl.h
@@ -59,7 +59,6 @@ typedef enum
DSM_OP_CREATE,
DSM_OP_ATTACH,
DSM_OP_DETACH,
- DSM_OP_RESIZE,
DSM_OP_DESTROY
} dsm_op;
--
2.17.0
0002-Remove-dsm_resize-and-dsm_remap.patchapplication/octet-stream; name=0002-Remove-dsm_resize-and-dsm_remap.patchDownload
From 9294197a50b16053776c39471120c9660c115460 Mon Sep 17 00:00:00 2001
From: Thomas Munro <thomas.munro@enterprisedb.com>
Date: Tue, 25 Sep 2018 15:51:13 +1200
Subject: [PATCH 2/2] Remove dsm_resize() and dsm_remap().
An earlier commit disabled them in a way that is friendly to client
code (if there is any), for back-branches. This commit removes them
completely from future releases.
Author: Thomas Munro
Reviewed-by:
Discussion: https://postgr.es/m/CAA4eK1%2B%3DyAFUvpFoHXFi_gm8YqmXN-TtkFH%2BVYjvDLS6-SFq-Q%40mail.gmail.com
---
src/backend/storage/ipc/dsm.c | 20 --------------------
src/backend/storage/ipc/dsm_impl.c | 13 -------------
src/include/storage/dsm.h | 4 +---
src/include/storage/dsm_impl.h | 3 ---
4 files changed, 1 insertion(+), 39 deletions(-)
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index af1162c6e8..a81e4db435 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -653,26 +653,6 @@ dsm_detach_all(void)
&dsm_control_mapped_size, ERROR);
}
-/*
- * Resize an existing shared memory segment.
- */
-void *
-dsm_resize(dsm_segment *seg, Size size)
-{
- elog(ERROR, "dsm_resize not supported");
- return NULL;
-}
-
-/*
- * Remap an existing shared memory segment.
- */
-void *
-dsm_remap(dsm_segment *seg)
-{
- elog(ERROR, "dsm_remap not supported");
- return NULL;
-}
-
/*
* Detach from a shared memory segment, destroying the segment if we
* remove the last reference.
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index fe581afffb..78154010aa 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -192,19 +192,6 @@ dsm_impl_op(dsm_op op, dsm_handle handle, Size request_size,
}
}
-/*
- * Previously we supported resizing DSM segments on some platforms. Since
- * the work was never extended to all platforms so that it couldn't be relied
- * on in general, and potential bugs were discovered on one one platform
- * where we did support it, we decided to remove this. The interface will
- * disappear completely in a later release.
- */
-bool
-dsm_impl_can_resize(void)
-{
- return false;
-}
-
#ifdef USE_DSM_POSIX
/*
* Operating system primitives to support POSIX shared memory.
diff --git a/src/include/storage/dsm.h b/src/include/storage/dsm.h
index 169de946f7..b4654cb5ca 100644
--- a/src/include/storage/dsm.h
+++ b/src/include/storage/dsm.h
@@ -33,11 +33,9 @@ extern void dsm_detach_all(void);
extern void dsm_set_control_handle(dsm_handle h);
#endif
-/* Functions that create, update, or remove mappings. */
+/* Functions that create or remove mappings. */
extern dsm_segment *dsm_create(Size size, int flags);
extern dsm_segment *dsm_attach(dsm_handle h);
-extern void *dsm_resize(dsm_segment *seg, Size size);
-extern void *dsm_remap(dsm_segment *seg);
extern void dsm_detach(dsm_segment *seg);
/* Resource management functions. */
diff --git a/src/include/storage/dsm_impl.h b/src/include/storage/dsm_impl.h
index c8adc425fd..9485446c91 100644
--- a/src/include/storage/dsm_impl.h
+++ b/src/include/storage/dsm_impl.h
@@ -67,9 +67,6 @@ extern bool dsm_impl_op(dsm_op op, dsm_handle handle, Size request_size,
void **impl_private, void **mapped_address, Size *mapped_size,
int elevel);
-/* Some implementations cannot resize segments. Can this one? */
-extern bool dsm_impl_can_resize(void);
-
/* Implementation-dependent actions required to keep segment until shutdown. */
extern void dsm_impl_pin_segment(dsm_handle handle, void *impl_private,
void **impl_private_pm_handle);
--
2.17.0
On Tue, Sep 25, 2018 at 06:22:19PM +1200, Thomas Munro wrote:
On Sat, Sep 22, 2018 at 4:52 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
I went through and check the original proposal [1] to see if any use
case is mentioned there, but nothing related has been discussed. I
couldn't think of much use of this facility except maybe for something
like parallelizing correalated sub-queries where the size of outer var
can change across executions and we might need to resize the initially
allocated memory. This is just a wild thought, I don't have any
concrete idea about this. Having said that, I don't object to
removing this especially because the implementation doesn't seem to be
complete. In future, if someone needs such a facility, they can first
develop a complete version of this API.Thanks for looking into that. Here's a pair of draft patches to
disable and then remove dsm_resize() and dsm_map().
Hm. Don't we need to worry about anybody potentially using these APIs
in a custom module on platforms where it was actually working? I
imagine that their reaction is not going be nice if any code breaks
suddenly after a minor release. No issues with removing the interface
on HEAD of course.
--
Michael
On Mon, Nov 5, 2018 at 2:28 AM Michael Paquier <michael@paquier.xyz> wrote:
Hm. Don't we need to worry about anybody potentially using these APIs
in a custom module on platforms where it was actually working? I
imagine that their reaction is not going be nice if any code breaks
suddenly after a minor release. No issues with removing the interface
on HEAD of course.
+1.
The fact that the code exists there at all is my fault. I thought it
might be useful someday, but now I don't think so any more. Thomas's
solution -- in the DSA machinery -- of allocating entirely new
segments seems like a better approach for now, and in the long run, I
think we should convert the whole backend to use threads,
nonwithstanding the TODO entry that says otherwise. Even if we never
do that, extending a segment in place is pretty difficult to make
practical, since it may involve remapping the segment, which
invalidates cached pointers to anything in the segment. And then
there are the portability problems on top of that. So I'm not very
optimistic about this any more.
But ripping it out in the back branches seems unnecessary. I'd just
leave the bugs unfixed there. Most likely nobody is using that stuff,
but if they are, let's not break it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, Nov 6, 2018 at 10:18 AM Robert Haas <robertmhaas@gmail.com> wrote:
On Mon, Nov 5, 2018 at 2:28 AM Michael Paquier <michael@paquier.xyz> wrote:
Hm. Don't we need to worry about anybody potentially using these APIs
in a custom module on platforms where it was actually working? I
imagine that their reaction is not going be nice if any code breaks
suddenly after a minor release. No issues with removing the interface
on HEAD of course.+1.
The fact that the code exists there at all is my fault. I thought it
might be useful someday, but now I don't think so any more. Thomas's
solution -- in the DSA machinery -- of allocating entirely new
segments seems like a better approach for now, and in the long run, I
think we should convert the whole backend to use threads,
nonwithstanding the TODO entry that says otherwise. Even if we never
do that, extending a segment in place is pretty difficult to make
practical, since it may involve remapping the segment, which
invalidates cached pointers to anything in the segment. And then
there are the portability problems on top of that. So I'm not very
optimistic about this any more.But ripping it out in the back branches seems unnecessary. I'd just
leave the bugs unfixed there. Most likely nobody is using that stuff,
but if they are, let's not break it.
Thanks. Pushed to master only.
--
Thomas Munro
http://www.enterprisedb.com
On Tue, Nov 06, 2018 at 05:29:36PM +1300, Thomas Munro wrote:
Thanks. Pushed to master only.
Just a wild idea while this thread is hot: could we add in the
description of the broken APIs or in their headers more information
about how to not use them so as users are warned if trying to use them
in certain circumstances? This idea is just for the stable branches.
--
Michael
On Tue, Nov 6, 2018 at 6:15 PM Michael Paquier <michael@paquier.xyz> wrote:
On Tue, Nov 06, 2018 at 05:29:36PM +1300, Thomas Munro wrote:
Thanks. Pushed to master only.
Just a wild idea while this thread is hot: could we add in the
description of the broken APIs or in their headers more information
about how to not use them so as users are warned if trying to use them
in certain circumstances? This idea is just for the stable branches.
Like this?
--
Thomas Munro
http://www.enterprisedb.com
Attachments:
deprecated.patchapplication/octet-stream; name=deprecated.patchDownload
diff --git a/src/backend/storage/ipc/dsm.c b/src/backend/storage/ipc/dsm.c
index f1f75b73f5..08b6fa8155 100644
--- a/src/backend/storage/ipc/dsm.c
+++ b/src/backend/storage/ipc/dsm.c
@@ -673,6 +673,8 @@ dsm_detach_all(void)
*
* This may cause the shared memory segment to be remapped at a different
* address. For the caller's convenience, we return the mapped address.
+ *
+ * This interface is deprecated and will be removed in a future release.
*/
void *
dsm_resize(dsm_segment *seg, Size size)
@@ -690,6 +692,8 @@ dsm_resize(dsm_segment *seg, Size size)
* mapping using dsm_resize(), but we've still only got the initial
* portion mapped. Since this might change the address at which the
* segment is mapped, we return the new mapped address.
+ *
+ * This interface is deprecated and will be removed in a future release.
*/
void *
dsm_remap(dsm_segment *seg)
diff --git a/src/backend/storage/ipc/dsm_impl.c b/src/backend/storage/ipc/dsm_impl.c
index b79701d837..0b0874779b 100644
--- a/src/backend/storage/ipc/dsm_impl.c
+++ b/src/backend/storage/ipc/dsm_impl.c
@@ -205,6 +205,8 @@ dsm_impl_op(dsm_op op, dsm_handle handle, Size request_size,
* since AIX allows shmctl(shmid, SHM_RESIZE, &buffer), though you apparently
* can't resize segments to anything larger than 256MB that way. For now,
* we keep it simple.)
+ *
+ * This interface is deprecated and will be removed in a future release.
*/
bool
dsm_impl_can_resize(void)
diff --git a/src/include/storage/dsm.h b/src/include/storage/dsm.h
index 169de946f7..f5f77b017a 100644
--- a/src/include/storage/dsm.h
+++ b/src/include/storage/dsm.h
@@ -36,8 +36,8 @@ extern void dsm_set_control_handle(dsm_handle h);
/* Functions that create, update, or remove mappings. */
extern dsm_segment *dsm_create(Size size, int flags);
extern dsm_segment *dsm_attach(dsm_handle h);
-extern void *dsm_resize(dsm_segment *seg, Size size);
-extern void *dsm_remap(dsm_segment *seg);
+extern void *dsm_resize(dsm_segment *seg, Size size); /* deprecated */
+extern void *dsm_remap(dsm_segment *seg); /* deprecated */
extern void dsm_detach(dsm_segment *seg);
/* Resource management functions. */
diff --git a/src/include/storage/dsm_impl.h b/src/include/storage/dsm_impl.h
index 0e5730f7c5..89a0ea82b7 100644
--- a/src/include/storage/dsm_impl.h
+++ b/src/include/storage/dsm_impl.h
@@ -70,7 +70,7 @@ extern bool dsm_impl_op(dsm_op op, dsm_handle handle, Size request_size,
int elevel);
/* Some implementations cannot resize segments. Can this one? */
-extern bool dsm_impl_can_resize(void);
+extern bool dsm_impl_can_resize(void); /* deprecated */
/* Implementation-dependent actions required to keep segment until shutdown. */
extern void dsm_impl_pin_segment(dsm_handle handle, void *impl_private,
On Tue, Nov 06, 2018 at 06:45:02PM +1300, Thomas Munro wrote:
Like this?
Ah, my mistake. I thought that the limitations with dsm_resize were not
documented but those actually return an error if trying to use
DSM_OP_RESIZE and I did not notice that, so we may be fine without a
comment or such in back-branches. Sorry for the noise.
--
Michael