> On 17 Jun 2018, at 14:47, Michael Paquier <mich...@paquier.xyz> wrote: > > On Thu, Jun 14, 2018 at 08:14:54PM +0000, Bossart, Nathan wrote: >> I'll go ahead and mark this as Ready for Committer. > > Another thing not mentioned on this thread is that bms_membership is > faster than bms_num_members by design with many members, so this change > makes sense to shave a couple of cycles. > > /* > * bms_num_members - count members of set > + * > + * In situations where the exact count isn't required, bms_membership can be > + * used to test if the set has 0, 1 or multiple members. > */ > bms_membership is a couple of lines down, I am not sure I see much point > in duplicating what's already present.
It is indeed a bit of a duplication, but the only reason this patch came to be was that the original author of the instances in postgres_fdw had missed said comment on bms_membership a few lines down. > - if (bms_num_members(clauses_attnums) < 2) > + if (bms_membership(clauses_attnums) != BMS_MULTIPLE) > For this one, the comment above directly mentions that at least two > attnums need to be present, so it seems to me that the current coding is > easier to understand and intentional... So I would be incline to not > change it. I don’t have any strong feelings either way, and will leave that call to the committer who picks this up. I agree that the current coding is easy to understand but I don’t see this being much harder. > I think that this should not go into the tree until REL_11_STABLE gets > created though, so this will have to wait a bit. 100% agree. cheers ./daniel
