> On Jan 3, 2026, at 18:50, David Rowley <[email protected]> wrote:
>
> On Mon, 29 Dec 2025 at 22:00, Chao Li <[email protected]> wrote:
>> I noticed this problem while reviewing Tom’s patch [1]. The function
>> eqjoinsel() unnecessarily zero-out sslot1 and sslot2 before 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, we should 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 code again, 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 backing changes 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/
