> On Jan 3, 2026, at 18:50, David Rowley <dgrowleyml@gmail.com> wrote:
>
> On Mon, 29 Dec 2025 at 22:00, Chao Li <li.evan.chao@gmail.com> wrote:
>> I noticed this problem while reviewing Tom’s patch [1]. The function eqjoinsel() unnecessarily zero-out sslot1 and
sslot2before calling get_attstatsslot() because get_attstatsslot() will unconditionally zero-out sslot in the first
place.
>>
>> I searched over the source tree, none of other callers of get_attstatsslot() initialize sslot. So for consistency,
weshould remove the unnecessary zero-out from eqjoinsel().
>
> I looked at this and was confused at why you think they're not needed.
> To check if I was misreading the code, I went on and applied your
> patch and tried running the tests. I get an immediate crash. You may
> not be aware that not all stats have MCVs lists. I believe there needs
> to be at least one duplicate value found in the sampled rows for the
> MCV list to be generated.
>
> I know you didn't ask me for tips, but I feel the need to provide one
> in the hope that at least something good comes out of this; if you're
> looking for something to optimise in your patch work, you should
> always pick quality over quantity. If too much poor-quality work
> arrives here, you're more likely to be ignored than respected. IMO,
> anyone who makes it to the ignore list of too many committers isn't
> going to have a high success rate of having their patches committed.
> Personally, I'd want to stay as far away from that as possible. We're
> quite bottlenecked on committer bandwidth already, so the best thing
> patch authors can do is make the committers job as easy as possible.
> If your patches are a pleasure to commit, then you're more likely to
> receive committer attention for your next patch.
>
> Now, as for the patch, you *could* work a bit harder and still get rid
> of the redundant zeroing, but I really doubt it's worth the effort.
> The struct is 64 bytes. Tiny memsets like this will be inlined by the
> compiler and zeroing 64 bytes amounts to a handful of mmx instructions
> on x86 with the default -march flags and likely fewer if you use a
> more modern -march version. If you believe these functions are hot
> enough for this to matter, then please provide proof in the form of
> benchmarks.
>
> Otherwise, I recommend going and frying some bigger fish.
>
> David
Hi David,
Sorry for this too-quick patch. I tried to post it before closing the day and forgot to run the tests. Looking at the
codeagain, get_attstatsslot() is only called under the && condition, so sslot is not always initialized without the
memset().That’s entirely on me.
I appreciate the advice on prioritizing quality over quantity. I’ll be more careful about validating assumptions and
backingchanges with tests before posting patches. Going forward, I’ll spend more time on each patch, both reviewing and
authoring.
Thanks again for the guidance.
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
