On Fri, Apr 8, 2022 at 8:45 PM Amit Langote <amitlangote09@gmail.com> wrote: > Most looked fine changes to me except a couple of typos, so I've > adopted those into the attached new version, even though I know it's > too late to try to apply it. > > + * XXX is it worth doing a bms_copy() on glob->minLockRelids if > + * glob->containsInitialPruning is true?. I'm slighly worried that the > + * Bitmapset could have a very long empty tail resulting in excessive > + * looping during AcquireExecutorLocks(). > + */ > > I guess I trust your instincts about bitmapset operation efficiency > and what you've written here makes sense. It's typical for leaf > partitions to have been appended toward the tail end of rtable and I'd > imagine their indexes would be in the tail words of minLockRelids. If > copying the bitmapset removes those useless words, I don't see why we > shouldn't do that. So added: > > + /* > + * It seems worth doing a bms_copy() on glob->minLockRelids if we deleted > + * bit from it just above to prevent empty tail bits resulting in > + * inefficient looping during AcquireExecutorLocks(). > + */ > + if (glob->containsInitialPruning) > + glob->minLockRelids = bms_copy(glob->minLockRelids) > > Not 100% about the comment I wrote.
And the quoted code change missed a semicolon in the v14 that I hurriedly sent on Friday. (Had apparently forgotten to `git add` the hunk to fix that).
Sending v15 that fixes that to keep the cfbot green for now.
