Hi,
On 2013-07-24 11:49:20 -0400, Tom Lane wrote:
> Andres Freund <and...@2ndquadrant.com> writes:
> > This will require another member variable in slist_mutable_iter which
> > obviously will need to be maintained, but that seems fine to me since it
> > will reduce the cost of actually deleting noticeably.
>
> I think that's all right. Conceivably we could introduce two forms of
> iterator depending on whether you want to delete or not, but that seems
> like overkill to me.
Agreed. Especially as you point out there's no real point to
mutable_iter as committed.
> In fact, now that I think about it, the distinction between slist_iter
> and slist_mutable_iter is really misstated in the comments, isn't it?
> The *only* action on the list that's unsafe with an active slist_iter
> is to delete the current element (and then continue iterating).
> If the value of slist_mutable_iter is to support deletion of the current
> element, then it seems obvious that it should support doing so
> efficiently, ie it should carry both prev and next. Also, if it's
> carrying both of those, then use of slist_mutable_iter really doesn't
> allow any action other than deleting the current element --- adding
> new nodes "ahead" of the current element isn't safe.
True. I think I mentally copy&pasted to much logic from the doubly
linked list case.
The first attached patch adds slist_delete_current(), updates the
comments addressing your points and converts the bgworker code to pass
the iterator around (it's more efficient which might actually matter
with a few hundred bgworkers).
I found the added newlines in slist_foreach_modify useful, but maybe they
should be removed again.
I think this should be included in 9.3 once reviewed.
The second patch adds a regression test for background workers via
worker_spi which I used to test slist_delete_current() addition. It's not 100%
as
it, but I thought it worthwile to post it anyway
a) only tests dynamically registered workers, it should start it's own
regression test starting some bgworkers statically
b) disables installcheck harshly causing a warning from make:
/home/andres/src/postgresql/src/makefiles/pgxs.mk:297: warning: ignoring old
commands for target `installcheck'
Manually defining a pg_regress in 'check:' should fix it probably
because that part of pgxs.mk is dependant on REGRESS = being set.
c) it only tests BGW_NEVER_RESTART
Greetings,
Andres Freund
--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
>From 0dfa542f457ebd71640830a993e3a428720b4179 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Wed, 24 Jul 2013 19:34:10 +0200
Subject: [PATCH 1/2] Add slist_delete_current()
---
src/backend/lib/ilist.c | 2 +-
src/backend/postmaster/bgworker.c | 5 ++++-
src/backend/postmaster/postmaster.c | 4 ++--
src/include/lib/ilist.h | 32 +++++++++++++++++++++++++----
src/include/postmaster/bgworker_internals.h | 2 +-
5 files changed, 36 insertions(+), 9 deletions(-)
diff --git a/src/backend/lib/ilist.c b/src/backend/lib/ilist.c
index 957a118..dddb6eb 100644
--- a/src/backend/lib/ilist.c
+++ b/src/backend/lib/ilist.c
@@ -28,7 +28,7 @@
*
* It is not allowed to delete a 'node' which is is not in the list 'head'
*
- * Caution: this is O(n)
+ * Caution: this is O(n), use slist_delete_current() while iterating.
*/
void
slist_delete(slist_head *head, slist_node *node)
diff --git a/src/backend/postmaster/bgworker.c b/src/backend/postmaster/bgworker.c
index ada24e9..9967b69 100644
--- a/src/backend/postmaster/bgworker.c
+++ b/src/backend/postmaster/bgworker.c
@@ -272,10 +272,13 @@ BackgroundWorkerStateChange(void)
* the postmaster.
*/
void
-ForgetBackgroundWorker(RegisteredBgWorker *rw)
+ForgetBackgroundWorker(slist_mutable_iter *cur)
{
+ RegisteredBgWorker *rw;
BackgroundWorkerSlot *slot;
+ rw = slist_container(RegisteredBgWorker, rw_lnode, cur->cur);
+
Assert(rw->rw_shmem_slot < max_worker_processes);
slot = &BackgroundWorkerData->slot[rw->rw_shmem_slot];
slot->in_use = false;
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index b6b8111..0342be7 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -1452,7 +1452,7 @@ DetermineSleepTime(struct timeval * timeout)
if (rw->rw_worker.bgw_restart_time == BGW_NEVER_RESTART)
{
- ForgetBackgroundWorker(rw);
+ ForgetBackgroundWorker(&siter);
continue;
}
@@ -5643,7 +5643,7 @@ StartOneBackgroundWorker(void)
{
if (rw->rw_worker.bgw_restart_time == BGW_NEVER_RESTART)
{
- ForgetBackgroundWorker(rw);
+ ForgetBackgroundWorker(&iter);
continue;
}
diff --git a/src/include/lib/ilist.h b/src/include/lib/ilist.h
index 73f4115..f8f0458 100644
--- a/src/include/lib/ilist.h
+++ b/src/include/lib/ilist.h
@@ -211,7 +211,9 @@ typedef struct slist_head
* Used as state in slist_foreach(). To get the current element of the
* iteration use the 'cur' member.
*
- * Do *not* manipulate the list while iterating!
+ * It's allowed to modify the list while iterating, with the sole exception of
+ * deleting the iterator's current node which may not be deleted unless the
+ * iteration is not continued afterwards.
*
* NB: this wouldn't really need to be an extra struct, we could use a
* slist_node * directly. We prefer a separate type for consistency.
@@ -226,11 +228,12 @@ typedef struct slist_iter
*
* Used as state in slist_foreach_modify().
*
- * Iterations using this are allowed to remove the current node and to add
- * more nodes ahead of the current node.
+ * The only modification allowed while iterating is to remove the current node
+ * either via slist_delete() or slist_delete_current().
*/
typedef struct slist_mutable_iter
{
+ slist_node *prev; /* prev node, for deletions */
slist_node *cur; /* current element */
slist_node *next; /* next node we'll iterate to */
} slist_mutable_iter;
@@ -243,7 +246,7 @@ typedef struct slist_mutable_iter
/* Prototypes for functions too big to be inline */
-/* Caution: this is O(n) */
+/* Caution: this is O(n), use slist_delete_current() while iterating */
extern void slist_delete(slist_head *head, slist_node *node);
#ifdef ILIST_DEBUG
@@ -578,6 +581,7 @@ extern slist_node *slist_pop_head_node(slist_head *head);
extern bool slist_has_next(slist_head *head, slist_node *node);
extern slist_node *slist_next_node(slist_head *head, slist_node *node);
extern slist_node *slist_head_node(slist_head *head);
+extern void slist_delete_current(slist_mutable_iter *cur);
/* slist macro support function */
extern void *slist_head_element_off(slist_head *head, size_t off);
@@ -679,6 +683,22 @@ slist_head_node(slist_head *head)
{
return (slist_node *) slist_head_element_off(head, 0);
}
+
+/*
+ * Delete the iterator's current element while preserving integrity of the
+ * underlying list.
+ */
+STATIC_IF_INLINE void
+slist_delete_current(slist_mutable_iter *cur)
+{
+ /*
+ * cur->prev will point to the slist_head's 'head' field when the iteration
+ * is at the first element of the list allowing us to treat perform
+ * deletion of the first element without a special case.
+ */
+ cur->prev->next = cur->next;
+}
+
#endif /* PG_USE_INLINE || ILIST_INCLUDE_DEFINITIONS */
/*
@@ -726,9 +746,13 @@ slist_head_node(slist_head *head)
#define slist_foreach_modify(iter, lhead) \
for (AssertVariableIsOfTypeMacro(iter, slist_mutable_iter), \
AssertVariableIsOfTypeMacro(lhead, slist_head *), \
+ (iter).prev = &(lhead)->head, \
(iter).cur = (lhead)->head.next, \
(iter).next = (iter).cur ? (iter).cur->next : NULL; \
+ \
(iter).cur != NULL; \
+ \
+ (iter).prev = (iter).cur, \
(iter).cur = (iter).next, \
(iter).next = (iter).next ? (iter).next->next : NULL)
diff --git a/src/include/postmaster/bgworker_internals.h b/src/include/postmaster/bgworker_internals.h
index 6484cfb..478a1ff 100644
--- a/src/include/postmaster/bgworker_internals.h
+++ b/src/include/postmaster/bgworker_internals.h
@@ -39,7 +39,7 @@ extern slist_head BackgroundWorkerList;
extern Size BackgroundWorkerShmemSize(void);
extern void BackgroundWorkerShmemInit(void);
extern void BackgroundWorkerStateChange(void);
-extern void ForgetBackgroundWorker(RegisteredBgWorker *);
+extern void ForgetBackgroundWorker(slist_mutable_iter *cur);
#ifdef EXEC_BACKEND
extern BackgroundWorker *BackgroundWorkerEntry(int slotno);
--
1.8.3.251.g1462b67
>From 2e63d0fece895eb7a662788fc3d4b3839cf29321 Mon Sep 17 00:00:00 2001
From: Andres Freund <and...@anarazel.de>
Date: Wed, 24 Jul 2013 19:34:29 +0200
Subject: [PATCH 2/2] Add regression tests for background workers via
worker_spi.
---
contrib/worker_spi/Makefile | 6 +
contrib/worker_spi/expected/worker_spi.out | 216 +++++++++++++++++++++++++++++
contrib/worker_spi/sql/worker_spi.sql | 44 ++++++
3 files changed, 266 insertions(+)
create mode 100644 contrib/worker_spi/expected/worker_spi.out
create mode 100644 contrib/worker_spi/sql/worker_spi.sql
diff --git a/contrib/worker_spi/Makefile b/contrib/worker_spi/Makefile
index fbb29b4..fcacf30 100644
--- a/contrib/worker_spi/Makefile
+++ b/contrib/worker_spi/Makefile
@@ -5,6 +5,8 @@ MODULES = worker_spi
EXTENSION = worker_spi
DATA = worker_spi--1.0.sql
+REGRESS = worker_spi
+
ifdef USE_PGXS
PG_CONFIG = pg_config
PGXS := $(shell $(PG_CONFIG) --pgxs)
@@ -15,3 +17,7 @@ top_builddir = ../..
include $(top_builddir)/src/Makefile.global
include $(top_srcdir)/contrib/contrib-global.mk
endif
+
+# Disabled, so we don't steal worker slots from a running instance.
+# Also, the tests depend on some default configuration variables.
+installcheck:;
diff --git a/contrib/worker_spi/expected/worker_spi.out b/contrib/worker_spi/expected/worker_spi.out
new file mode 100644
index 0000000..e11f453
--- /dev/null
+++ b/contrib/worker_spi/expected/worker_spi.out
@@ -0,0 +1,216 @@
+-- hardcoded in worker_spi atm
+\c postgres
+CREATE EXTENSION worker_spi;
+-- default's still the same
+SHOW max_worker_processes;
+ max_worker_processes
+----------------------
+ 8
+(1 row)
+
+SELECT worker_spi_launch(1);
+ worker_spi_launch
+-------------------
+ t
+(1 row)
+
+SELECT worker_spi_launch(2);
+ worker_spi_launch
+-------------------
+ t
+(1 row)
+
+SELECT worker_spi_launch(3);
+ worker_spi_launch
+-------------------
+ t
+(1 row)
+
+SELECT worker_spi_launch(4);
+ worker_spi_launch
+-------------------
+ t
+(1 row)
+
+SELECT worker_spi_launch(5);
+ worker_spi_launch
+-------------------
+ t
+(1 row)
+
+SELECT worker_spi_launch(6);
+ worker_spi_launch
+-------------------
+ t
+(1 row)
+
+SELECT worker_spi_launch(7);
+ worker_spi_launch
+-------------------
+ t
+(1 row)
+
+SELECT worker_spi_launch(8);
+ worker_spi_launch
+-------------------
+ t
+(1 row)
+
+-- should fail
+SELECT worker_spi_launch(9);
+ worker_spi_launch
+-------------------
+ f
+(1 row)
+
+-- wait till it started
+SELECT pg_sleep(3);
+ pg_sleep
+----------
+
+(1 row)
+
+SELECT count(*) FROM pg_stat_activity WHERE application_name NOT IN ('psql', 'pg_regress');
+ count
+-------
+ 8
+(1 row)
+
+\dn
+ List of schemas
+ Name | Owner
+---------+--------
+ public | andres
+ schema1 | andres
+ schema2 | andres
+ schema3 | andres
+ schema4 | andres
+ schema5 | andres
+ schema6 | andres
+ schema7 | andres
+ schema8 | andres
+(9 rows)
+
+\dt schema*.*
+ List of relations
+ Schema | Name | Type | Owner
+---------+---------+-------+--------
+ schema1 | counted | table | andres
+ schema2 | counted | table | andres
+ schema3 | counted | table | andres
+ schema4 | counted | table | andres
+ schema5 | counted | table | andres
+ schema6 | counted | table | andres
+ schema7 | counted | table | andres
+ schema8 | counted | table | andres
+(8 rows)
+
+SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE application_name NOT IN ('psql', 'pg_regress');
+ pg_terminate_backend
+----------------------
+ t
+ t
+ t
+ t
+ t
+ t
+ t
+ t
+(8 rows)
+
+SELECT pg_sleep(1);
+ pg_sleep
+----------
+
+(1 row)
+
+SELECT count(*) FROM pg_stat_activity WHERE application_name NOT IN ('psql', 'pg_regress');
+ count
+-------
+ 0
+(1 row)
+
+-- can start after crash
+SELECT worker_spi_launch(10);
+ worker_spi_launch
+-------------------
+ t
+(1 row)
+
+SELECT worker_spi_launch(11);
+ worker_spi_launch
+-------------------
+ t
+(1 row)
+
+-- wait till started
+SELECT pg_sleep(3);
+ pg_sleep
+----------
+
+(1 row)
+
+SELECT count(*) FROM pg_stat_activity WHERE application_name NOT IN ('psql', 'pg_regress');
+ count
+-------
+ 2
+(1 row)
+
+SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE application_name NOT IN ('psql', 'pg_regress');
+ pg_terminate_backend
+----------------------
+ t
+ t
+(2 rows)
+
+SELECT pg_sleep(3);
+ pg_sleep
+----------
+
+(1 row)
+
+SELECT count(*) FROM pg_stat_activity WHERE application_name NOT IN ('psql', 'pg_regress');
+ count
+-------
+ 0
+(1 row)
+
+SELECT pg_sleep(3);
+ pg_sleep
+----------
+
+(1 row)
+
+\dn
+ List of schemas
+ Name | Owner
+----------+--------
+ public | andres
+ schema1 | andres
+ schema10 | andres
+ schema11 | andres
+ schema2 | andres
+ schema3 | andres
+ schema4 | andres
+ schema5 | andres
+ schema6 | andres
+ schema7 | andres
+ schema8 | andres
+(11 rows)
+
+\dt schema*.*
+ List of relations
+ Schema | Name | Type | Owner
+----------+---------+-------+--------
+ schema1 | counted | table | andres
+ schema10 | counted | table | andres
+ schema11 | counted | table | andres
+ schema2 | counted | table | andres
+ schema3 | counted | table | andres
+ schema4 | counted | table | andres
+ schema5 | counted | table | andres
+ schema6 | counted | table | andres
+ schema7 | counted | table | andres
+ schema8 | counted | table | andres
+(10 rows)
+
diff --git a/contrib/worker_spi/sql/worker_spi.sql b/contrib/worker_spi/sql/worker_spi.sql
new file mode 100644
index 0000000..41664a2
--- /dev/null
+++ b/contrib/worker_spi/sql/worker_spi.sql
@@ -0,0 +1,44 @@
+-- hardcoded in worker_spi atm
+\c postgres
+
+CREATE EXTENSION worker_spi;
+
+-- default's still the same
+SHOW max_worker_processes;
+
+SELECT worker_spi_launch(1);
+SELECT worker_spi_launch(2);
+SELECT worker_spi_launch(3);
+SELECT worker_spi_launch(4);
+SELECT worker_spi_launch(5);
+SELECT worker_spi_launch(6);
+SELECT worker_spi_launch(7);
+SELECT worker_spi_launch(8);
+-- should fail
+SELECT worker_spi_launch(9);
+
+-- wait till it started
+SELECT pg_sleep(3);
+SELECT count(*) FROM pg_stat_activity WHERE application_name NOT IN ('psql', 'pg_regress');
+\dn
+\dt schema*.*
+
+SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE application_name NOT IN ('psql', 'pg_regress');
+SELECT pg_sleep(1);
+SELECT count(*) FROM pg_stat_activity WHERE application_name NOT IN ('psql', 'pg_regress');
+
+-- can start after crash
+SELECT worker_spi_launch(10);
+SELECT worker_spi_launch(11);
+
+-- wait till started
+SELECT pg_sleep(3);
+SELECT count(*) FROM pg_stat_activity WHERE application_name NOT IN ('psql', 'pg_regress');
+
+SELECT pg_terminate_backend(pid) FROM pg_stat_activity WHERE application_name NOT IN ('psql', 'pg_regress');
+SELECT pg_sleep(3);
+SELECT count(*) FROM pg_stat_activity WHERE application_name NOT IN ('psql', 'pg_regress');
+
+SELECT pg_sleep(3);
+\dn
+\dt schema*.*
--
1.8.3.251.g1462b67
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
