Remove vardata parameters from eqjoinsel_inner

On Fri, Feb 21, 2025 at 7:04 PM Ilia Evdokimov
<ilya.evdokimov@tantorlabs.com> wrote:

When calculating selectivity for an inner equijoin, we call
eqjoinsel_inner, which uses unused parameters vardata1 and vardata2.
These parameters might have been left behind accidentally when we moved
getting sslots out of the function. I suggest removing them, as they can
be added back at any time if needed. I attached patch with fixes.

Yeah, these parameters haven't been used since a314c3407, when we
moved get_variable_numdistinct and get_attstatsslot out of
eqjoinsel_inner and eqjoinsel_semi to avoid repetitive information
lookup when we call both eqjoinsel_inner and eqjoinsel_semi.

I'm wondering whether we should also remove parameter vardata1 from
eqjoinsel_semi. vardata2 is still needed though to clamp nd2 to be
not more than the rel's row estimate.

Thanks
Richard

On 27.03.2025 10:48, Richard Guo wrote:

I'm wondering whether we should also remove parameter vardata1 from
eqjoinsel_semi. vardata2 is still needed though to clamp nd2 to be
not more than the rel's row estimate.

Thanks
Richard

Indeed, the parameter vardata1 in eqjoinsel_semi() is currently unused
and could logically be removed. However, simply leaving a single
parameter named vardata2 would appear strange and unintuitive, as it
implicitly suggests the existence of a corresponding "first" parameter.
I suggest renaming vardata2 to something more descriptive, such as
rhs_vardata, clearly indicating its role related specifically to the
right side of the join condition.

I attached v2 patch with changes.

Any thoughts?

--
Best regards,
Ilia Evdokimov,
Tantor Labs LLC.

Richard Guo <guofenglinux@gmail.com> writes:

On Fri, Feb 21, 2025 at 7:04 PM Ilia Evdokimov
<ilya.evdokimov@tantorlabs.com> wrote:

When calculating selectivity for an inner equijoin, we call
eqjoinsel_inner, which uses unused parameters vardata1 and vardata2.
These parameters might have been left behind accidentally when we moved
getting sslots out of the function. I suggest removing them, as they can
be added back at any time if needed. I attached patch with fixes.

Yeah, these parameters haven't been used since a314c3407, when we
moved get_variable_numdistinct and get_attstatsslot out of
eqjoinsel_inner and eqjoinsel_semi to avoid repetitive information
lookup when we call both eqjoinsel_inner and eqjoinsel_semi.

I'm wondering whether we should also remove parameter vardata1 from
eqjoinsel_semi. vardata2 is still needed though to clamp nd2 to be
not more than the rel's row estimate.

I do not believe this change is worth the code churn. In the first
place, we may well need those values again someday. In the second
place, the savings would be negligible. (In fact, since
eqjoinsel_inner probably gets inlined at its sole call site, the
savings would likely be completely nonexistent.) If the caller
could avoid calculating the vardata info at all, that would be
worth thinking about, but I think it can't.

regards, tom lane