Hi,
On 2013-07-24 11:49:20 -0400, Tom Lane wrote:
> Andres Freund <andres@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
