David Rowley <david.row...@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