A couple of cosmetic changes around shared memory code
Hello,
while investigating the shm_mq code and its testing module I made some
cosmetic improvements there. You can see them in the attached diff file.
Attachments:
Cosmetic-improvements-around-shm_mq-and-test_shm_mq.difftext/x-patch; name=Cosmetic-improvements-around-shm_mq-and-test_shm_mq.diffDownload
commit 0e202cb6e0eca2e7fb3e1353b550f3d2ace9680e
Author: Piotr Stefaniak <postgres@piotr-stefaniak.me>
Date: Thu Apr 28 18:36:16 2016 +0200
Cosmetic improvements around shm_mq and test_shm_mq.
diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c
index 7859f42..292d515 100644
--- a/src/backend/storage/ipc/shm_mq.c
+++ b/src/backend/storage/ipc/shm_mq.c
@@ -103,7 +103,7 @@ struct shm_mq
* locally by copying the chunks into a backend-local buffer. mqh_buffer is
* the buffer, and mqh_buflen is the number of bytes allocated for it.
*
- * mqh_partial_message_bytes, mqh_expected_bytes, and mqh_length_word_complete
+ * mqh_partial_bytes, mqh_expected_bytes, and mqh_length_word_complete
* are used to track the state of non-blocking operations. When the caller
* attempts a non-blocking operation that returns SHM_MQ_WOULD_BLOCK, they
* are expected to retry the call at a later time with the same argument;
diff --git a/src/backend/storage/ipc/shm_toc.c b/src/backend/storage/ipc/shm_toc.c
index 55248c2..e1d6bd1 100644
--- a/src/backend/storage/ipc/shm_toc.c
+++ b/src/backend/storage/ipc/shm_toc.c
@@ -96,7 +96,7 @@ shm_toc_allocate(shm_toc *toc, Size nbytes)
total_bytes = vtoc->toc_total_bytes;
allocated_bytes = vtoc->toc_allocated_bytes;
nentry = vtoc->toc_nentry;
- toc_bytes = offsetof(shm_toc, toc_entry) +nentry * sizeof(shm_toc_entry)
+ toc_bytes = offsetof(shm_toc, toc_entry) + nentry * sizeof(shm_toc_entry)
+ allocated_bytes;
/* Check for memory exhaustion and overflow. */
@@ -132,7 +132,7 @@ shm_toc_freespace(shm_toc *toc)
nentry = vtoc->toc_nentry;
SpinLockRelease(&toc->toc_mutex);
- toc_bytes = offsetof(shm_toc, toc_entry) +nentry * sizeof(shm_toc_entry);
+ toc_bytes = offsetof(shm_toc, toc_entry) + nentry * sizeof(shm_toc_entry);
Assert(allocated_bytes + BUFFERALIGN(toc_bytes) <= total_bytes);
return total_bytes - (allocated_bytes + BUFFERALIGN(toc_bytes));
}
@@ -176,7 +176,7 @@ shm_toc_insert(shm_toc *toc, uint64 key, void *address)
total_bytes = vtoc->toc_total_bytes;
allocated_bytes = vtoc->toc_allocated_bytes;
nentry = vtoc->toc_nentry;
- toc_bytes = offsetof(shm_toc, toc_entry) +nentry * sizeof(shm_toc_entry)
+ toc_bytes = offsetof(shm_toc, toc_entry) + nentry * sizeof(shm_toc_entry)
+ allocated_bytes;
/* Check for memory exhaustion and overflow. */
diff --git a/src/test/modules/test_shm_mq/setup.c b/src/test/modules/test_shm_mq/setup.c
index 5bd2820..a0f3962 100644
--- a/src/test/modules/test_shm_mq/setup.c
+++ b/src/test/modules/test_shm_mq/setup.c
@@ -125,7 +125,7 @@ setup_dynamic_shared_memory(int64 queue_size, int nworkers,
segsize = shm_toc_estimate(&e);
/* Create the shared memory segment and establish a table of contents. */
- seg = dsm_create(shm_toc_estimate(&e), 0);
+ seg = dsm_create(segsize, 0);
toc = shm_toc_create(PG_TEST_SHM_MQ_MAGIC, dsm_segment_address(seg),
segsize);
diff --git a/src/test/modules/test_shm_mq/worker.c b/src/test/modules/test_shm_mq/worker.c
index 638649b..a94414a 100644
--- a/src/test/modules/test_shm_mq/worker.c
+++ b/src/test/modules/test_shm_mq/worker.c
@@ -139,7 +139,7 @@ test_shm_mq_main(Datum main_arg)
* we can go ahead and exit.
*/
dsm_detach(seg);
- proc_exit(1);
+ proc_exit(0);
}
/*
On Tue, May 17, 2016 at 4:40 AM, Piotr Stefaniak
<postgres@piotr-stefaniak.me> wrote:
while investigating the shm_mq code and its testing module I made some
cosmetic improvements there. You can see them in the attached diff file.
- toc_bytes = offsetof(shm_toc, toc_entry) +nentry * sizeof(shm_toc_entry)
+ toc_bytes = offsetof(shm_toc, toc_entry) + nentry * sizeof(shm_toc_entry)
+ allocated_bytes;
I don't recall the exact reason, but this is intentional style
(memories from a patchwork with Tom). See for example geo_ops.c or
pl_funcs.c. Though it is true that things are not completely
consistent in the code with offset.- seg = dsm_create(shm_toc_estimate(&e), 0);
+ seg = dsm_create(segsize, 0);
Yep.- proc_exit(1);
+ proc_exit(0);
Agreed here. I don't see why this should not exit with 0 if there have
not been any errors.
--
Michael--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Michael Paquier <michael.paquier@gmail.com> writes:
On Tue, May 17, 2016 at 4:40 AM, Piotr Stefaniak <postgres@piotr-stefaniak.me> wrote: - toc_bytes = offsetof(shm_toc, toc_entry) +nentry * sizeof(shm_toc_entry) + toc_bytes = offsetof(shm_toc, toc_entry) + nentry * sizeof(shm_toc_entry) + allocated_bytes;
I don't recall the exact reason, but this is intentional style
(memories from a patchwork with Tom).
Well, it's not so much intentional as that pgindent will make it look like
that no matter what you do --- it's got some weird interaction with
sizeof, offsetof, and typedef names versus operators later on the same
line. I'd call that a pgindent bug myself, but have no particular desire
to try to fix it.
You could possibly make it look nicer by splitting into multiple lines,
but you'll need to try pgindent'ing to see if you've actually improved the
end result at all.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-05-17 19:05, Tom Lane wrote:
Michael Paquier <michael.paquier@gmail.com> writes:
On Tue, May 17, 2016 at 4:40 AM, Piotr Stefaniak <postgres@piotr-stefaniak.me> wrote: - toc_bytes = offsetof(shm_toc, toc_entry) +nentry * sizeof(shm_toc_entry) + toc_bytes = offsetof(shm_toc, toc_entry) + nentry * sizeof(shm_toc_entry) + allocated_bytes;I don't recall the exact reason, but this is intentional style
(memories from a patchwork with Tom).Well, it's not so much intentional as that pgindent will make it look like
that no matter what you do --- it's got some weird interaction with
sizeof, offsetof, and typedef names versus operators later on the same
line. I'd call that a pgindent bug myself, but have no particular desire
to try to fix it.
From my understanding, it's because pg_bsd_indent thinks that
"(shm_toc)" is a cast since shm_toc is a keyword (type alias fetched
from the "list of typedefs" file in this case, to be precise), forcing
the "+" to be a unary operator instead of binary.
One easy way to work around this bug is putting shm_toc into its own
parentheses but I'd prefer dropping this particular fix from my
"cosmetic changes" patch until pg_bsd_indent is fixed.
I may come up with a possible fix for this bug, but don't hold your breath.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-05-16 21:40, Piotr Stefaniak wrote:
Hello,
while investigating the shm_mq code and its testing module I made some
cosmetic improvements there. You can see them in the attached diff file.
Revised patch attached.
Attachments:
Cosmetic-improvements-around-shm_mq-and-test_shm_mq_v2.difftext/x-patch; name=Cosmetic-improvements-around-shm_mq-and-test_shm_mq_v2.diffDownload
commit 3ff1afa84e4bc22f153c876e2f0588327a8a004e
Author: Piotr Stefaniak <postgres@piotr-stefaniak.me>
Date: Thu Apr 28 18:36:16 2016 +0200
Cosmetic improvements around shm_mq and test_shm_mq.
diff --git a/src/backend/storage/ipc/shm_mq.c b/src/backend/storage/ipc/shm_mq.c
index cc1f04d..64a5475 100644
--- a/src/backend/storage/ipc/shm_mq.c
+++ b/src/backend/storage/ipc/shm_mq.c
@@ -103,7 +103,7 @@ struct shm_mq
* locally by copying the chunks into a backend-local buffer. mqh_buffer is
* the buffer, and mqh_buflen is the number of bytes allocated for it.
*
- * mqh_partial_message_bytes, mqh_expected_bytes, and mqh_length_word_complete
+ * mqh_partial_bytes, mqh_expected_bytes, and mqh_length_word_complete
* are used to track the state of non-blocking operations. When the caller
* attempts a non-blocking operation that returns SHM_MQ_WOULD_BLOCK, they
* are expected to retry the call at a later time with the same argument;
diff --git a/src/test/modules/test_shm_mq/setup.c b/src/test/modules/test_shm_mq/setup.c
index 5bd2820..a0f3962 100644
--- a/src/test/modules/test_shm_mq/setup.c
+++ b/src/test/modules/test_shm_mq/setup.c
@@ -125,7 +125,7 @@ setup_dynamic_shared_memory(int64 queue_size, int nworkers,
segsize = shm_toc_estimate(&e);
/* Create the shared memory segment and establish a table of contents. */
- seg = dsm_create(shm_toc_estimate(&e), 0);
+ seg = dsm_create(segsize, 0);
toc = shm_toc_create(PG_TEST_SHM_MQ_MAGIC, dsm_segment_address(seg),
segsize);
diff --git a/src/test/modules/test_shm_mq/worker.c b/src/test/modules/test_shm_mq/worker.c
index 638649b..a94414a 100644
--- a/src/test/modules/test_shm_mq/worker.c
+++ b/src/test/modules/test_shm_mq/worker.c
@@ -139,7 +139,7 @@ test_shm_mq_main(Datum main_arg)
* we can go ahead and exit.
*/
dsm_detach(seg);
- proc_exit(1);
+ proc_exit(0);
}
/*
On Sun, Jun 26, 2016 at 6:19 AM, Piotr Stefaniak
<postgres@piotr-stefaniak.me> wrote:
while investigating the shm_mq code and its testing module I made some
cosmetic improvements there. You can see them in the attached diff file.Revised patch attached.
The first hunk of this corrects an outdated comment, so we should
certainly apply that. I'm not seeing what the value of the other bits
is.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Jun 28, 2016 at 6:49 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Sun, Jun 26, 2016 at 6:19 AM, Piotr Stefaniak
<postgres@piotr-stefaniak.me> wrote:while investigating the shm_mq code and its testing module I made some
cosmetic improvements there. You can see them in the attached diff file.Revised patch attached.
The first hunk of this corrects an outdated comment, so we should
certainly apply that. I'm not seeing what the value of the other bits
is.
- proc_exit(1);
+ proc_exit(0);
Looking again at this thread with fresh eyes, isn't the origin of the
confusion the fact that we do need to have a non-zero error code so as
the worker is never restarted thanks to BGW_NEVER_RESTART? Even with
that, it is a strange concept to leave with proc_exit(1) in the case
where a worker left correctly..
--
Michael
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Mon, Jun 27, 2016 at 11:40 PM, Michael Paquier
<michael.paquier@gmail.com> wrote:
On Tue, Jun 28, 2016 at 6:49 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Sun, Jun 26, 2016 at 6:19 AM, Piotr Stefaniak
<postgres@piotr-stefaniak.me> wrote:while investigating the shm_mq code and its testing module I made some
cosmetic improvements there. You can see them in the attached diff file.Revised patch attached.
The first hunk of this corrects an outdated comment, so we should
certainly apply that. I'm not seeing what the value of the other bits
is.- proc_exit(1);
+ proc_exit(0);
Looking again at this thread with fresh eyes, isn't the origin of the
confusion the fact that we do need to have a non-zero error code so as
the worker is never restarted thanks to BGW_NEVER_RESTART? Even with
that, it is a strange concept to leave with proc_exit(1) in the case
where a worker left correctly..
This code predates be7558162acc5578d0b2cf0c8d4c76b6076ce352, prior to
which proc_exit(0) forced an immediate, unconditional restart. It's
true that, given that commit, changing this code to do proc_exit(0)
instead of proc_exit(1) would be harmless. However, people writing
background workers that might need to work with 9.3 would be best
advised to stick with proc_exit(1). Therefore, I maintain that this
is not broken and doesn't need to be fixed.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 2016-06-29 18:58, Robert Haas wrote:
This code predates be7558162acc5578d0b2cf0c8d4c76b6076ce352, prior to
which proc_exit(0) forced an immediate, unconditional restart. It's
true that, given that commit, changing this code to do proc_exit(0)
instead of proc_exit(1) would be harmless. However, people writing
background workers that might need to work with 9.3 would be best
advised to stick with proc_exit(1). Therefore, I maintain that this
is not broken and doesn't need to be fixed.
Then I'd argue that it ought to be documented in form of a C comment for
people writing background workers and for those who might want to "fix"
this in the future.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Jun 29, 2016 at 4:35 PM, Piotr Stefaniak
<postgres@piotr-stefaniak.me> wrote:
On 2016-06-29 18:58, Robert Haas wrote:
This code predates be7558162acc5578d0b2cf0c8d4c76b6076ce352, prior to
which proc_exit(0) forced an immediate, unconditional restart. It's
true that, given that commit, changing this code to do proc_exit(0)
instead of proc_exit(1) would be harmless. However, people writing
background workers that might need to work with 9.3 would be best
advised to stick with proc_exit(1). Therefore, I maintain that this
is not broken and doesn't need to be fixed.Then I'd argue that it ought to be documented in form of a C comment for
people writing background workers and for those who might want to "fix" this
in the future.
Well, I suppose we could do that. Would we then add the same comment
to worker_spi, which does the same thing for the same reason, and
every future contrib module that does stuff with background workers
which we might accept?
It might be better to document this in bgworker.sgml instead. That
already documents some facts about exiting:
<para>
If <structfield>bgw_restart_time</structfield> for a background worker is
configured as <literal>BGW_NEVER_RESTART</>, or if it exits with an exit
code of 0 or is terminated by <function>TerminateBackgroundWorker</>,
it will be automatically unregistered by the postmaster on exit.
Otherwise, it will be restarted after the time period configured via
<structfield>bgw_restart_time</>, or immediately if the postmaster
reinitializes the cluster due to a backend failure. Backends which need
to suspend execution only temporarily should use an interruptible sleep
rather than exiting; this can be achieved by calling
<function>WaitLatch()</function>. Make sure the
<literal>WL_POSTMASTER_DEATH</> flag is set when calling that function, and
verify the return code for a prompt exit in the emergency case that
<command>postgres</> itself has terminated.
</para>
That paragraph leaves out a number of important details, like:
1. A background worker that exits with any exit code other than 0 or 1
will cause a postmaster crash-and-restart cycle.
2. Therefore, an exit of code 1 should be used whenever the process
wants to be restarted in accordance with bgw_restart_time, and is
therefore in some sense the "normal" way for a background worker to
exit.
3. The aforementioned details about how 9.3 behavior was different
from current behavior.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Robert Haas wrote:
It might be better to document this in bgworker.sgml instead. That
already documents some facts about exiting:<para>
If <structfield>bgw_restart_time</structfield> for a background worker is
configured as <literal>BGW_NEVER_RESTART</>, or if it exits with an exit
code of 0 or is terminated by <function>TerminateBackgroundWorker</>,
it will be automatically unregistered by the postmaster on exit.
Otherwise, it will be restarted after the time period configured via
<structfield>bgw_restart_time</>, or immediately if the postmaster
reinitializes the cluster due to a backend failure. Backends which need
to suspend execution only temporarily should use an interruptible sleep
rather than exiting; this can be achieved by calling
<function>WaitLatch()</function>. Make sure the
<literal>WL_POSTMASTER_DEATH</> flag is set when calling that function, and
verify the return code for a prompt exit in the emergency case that
<command>postgres</> itself has terminated.
</para>That paragraph leaves out a number of important details, like:
1. A background worker that exits with any exit code other than 0 or 1
will cause a postmaster crash-and-restart cycle.2. Therefore, an exit of code 1 should be used whenever the process
wants to be restarted in accordance with bgw_restart_time, and is
therefore in some sense the "normal" way for a background worker to
exit.
Yeah, I think bgworker.sgml is the right place to document these
details.
3. The aforementioned details about how 9.3 behavior was different
from current behavior.
Not really sure about this. I think patching the 9.3 docs to state the
details differently from the 9.4--master details would not be
sufficient, as people moving code would not read the 9.4 docs again to
ensure the semantics remain the same (and since 9.4 has been out for
awhile, patching the release notes wouldn't suffice either). Patching
the 9.3 docs to say "9.4 is different" would be odd, since 9.4 is "in
the future" for the POV of 9.3 docs.
I think the best option is to backpatch a doc change from 9.4 onwards
stating what is the new behavior, and add a note stating that 9.3 was
different.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers