Re: ilist.h is not useful as-is

Hi,

On 2013-07-24 11:49:20 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)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