David Rowley <david.rowley@2ndquadrant.com> writes:
> If we're doing an API break for this, wouldn't it be better to come up
> with something that didn't have to shuffle list elements around every
> time one is deleted?
Agreed that as long as there's an API break anyway, we could consider
more extensive changes for this use-case. But ...
> For example, we could have a foreach_delete() that instead of taking a
> pointer to a ListCell, it took a ListDeleteIterator which contained a
> ListCell pointer and a Bitmapset, then just have a macro that marks a
> list item as deleted (list_delete_current(di)) and have a final
> cleanup at the end of the loop.
... I'm not quite sold on this particular idea. The amount of added
bitmapset manipulation overhead seems rather substantial in comparison
to the memmove work saved. It might win for cases involving very
long lists with many entries being deleted in one operation, but
I don't think that's a common scenario for us. It's definitely a
loss when there's just one item to be deleted, which I think is a
common case. (Of course, callers expecting that could just not
use this multi-delete API.)
> Or maybe we should worry about having the list in an inconsistent
> state during the loop? e.g if the list is getting passed into a
> function call to do something.
Not following that? If I understand your idea correctly, the list
doesn't actually get changed until the cleanup step. If we pass it to
another operation that independently deletes some members meanwhile,
that's trouble; but it'd be trouble for the existing code, and for my
version of the patch too.
FWIW, I don't really see a need to integrate this idea into the
loop logic as such. You could just define it as "make a bitmap
of the list indexes to delete, then call
"list = list_delete_multi(list, bitmapset)". It would be
helpful perhaps if we provided official access to the current
list index that the foreach macro is maintaining internally.
regards, tom lane
