Content-Type: multipart/mixed; boundary="------------Nqo9DALGO0un850G0z4IhyCu"
Message-ID: <95f1deab-3458-45f6-a13b-d6f12b3d3ba3@tantorlabs.com>
Date: Thu, 27 Mar 2025 14:00:30 +0300
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Remove vardata parameters from eqjoinsel_inner
To: Richard Guo <guofenglinux@gmail.com>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>
References: <7cdf4ecd-8789-4d85-ab59-2b78d4d27b1f@tantorlabs.com>
 <CAMbWs49ybFS2UPhm4veuTPJw8ZTsrjjXYQgE9Pi5neC2XvOrbg@mail.gmail.com>
Content-Language: en-US
From: Ilia Evdokimov <ilya.evdokimov@tantorlabs.com>
In-Reply-To: 
 <CAMbWs49ybFS2UPhm4veuTPJw8ZTsrjjXYQgE9Pi5neC2XvOrbg@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/95f1deab-3458-45f6-a13b-d6f12b3d3ba3%40tantorlabs.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------Nqo9DALGO0un850G0z4IhyCu
Content-Type: multipart/alternative;
 boundary="------------LrNkwLuMZBHxT1pgVNqOPJxW"

--------------LrNkwLuMZBHxT1pgVNqOPJxW
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit


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.

--------------LrNkwLuMZBHxT1pgVNqOPJxW
Content-Type: text/html; charset=UTF-8
Content-Transfer-Encoding: 7bit

<!DOCTYPE html>
<html>
  <head>
    <meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
  </head>
  <body>
    <p><br>
    </p>
    <div class="moz-cite-prefix">On 27.03.2025 10:48, Richard Guo wrote:<span
      style="white-space: pre-wrap">
</span></div>
    <blockquote type="cite"
cite="mid:CAMbWs49ybFS2UPhm4veuTPJw8ZTsrjjXYQgE9Pi5neC2XvOrbg@mail.gmail.com">
      <pre class="moz-quote-pre" wrap="">
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
</pre>
    </blockquote>
    <p><br>
    </p>
    <p>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.</p>
    <p>I attached v2 patch with changes.</p>
    <p>Any thoughts?<br>
    </p>
    <p>--<br>
      Best regards,<br>
      Ilia Evdokimov,<br>
      Tantor Labs LLC.<br>
    </p>
  </body>
</html>

--------------LrNkwLuMZBHxT1pgVNqOPJxW--

--------------Nqo9DALGO0un850G0z4IhyCu
Content-Type: text/x-patch; charset=UTF-8;
 name="v2-0001-Remove-unused-vardata-parameters-in-eqjoinsel.patch"
Content-Disposition: attachment;
 filename*0="v2-0001-Remove-unused-vardata-parameters-in-eqjoinsel.patch"
Content-Transfer-Encoding: base64

RnJvbSA2ZWM5OWQ3MTJkN2ZkYTNjYzE4YTg3MDAzMTgxNThmNTRiYWU0YzU1IE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBFdmRva2ltb3YgSWxpYSA8aWx5YS5ldmRva2ltb3ZA
dGFudG9ybGFicy5jb20+CkRhdGU6IFRodSwgMjcgTWFyIDIwMjUgMTM6NTk6MDIgKzAzMDAK
U3ViamVjdDogW1BBVENIIHYyXSBSZW1vdmUgdW51c2VkIHZhcmRhdGEgcGFyYW1ldGVycyBp
biBlcWpvaW5zZWxfaW5uZXIgYW5kCiBlcWpvaW5zZWxfc2VtaQoKLS0tCiBzcmMvYmFja2Vu
ZC91dGlscy9hZHQvc2VsZnVuY3MuYyB8IDE3ICsrKysrKystLS0tLS0tLS0tCiAxIGZpbGUg
Y2hhbmdlZCwgNyBpbnNlcnRpb25zKCspLCAxMCBkZWxldGlvbnMoLSkKCmRpZmYgLS1naXQg
YS9zcmMvYmFja2VuZC91dGlscy9hZHQvc2VsZnVuY3MuYyBiL3NyYy9iYWNrZW5kL3V0aWxz
L2FkdC9zZWxmdW5jcy5jCmluZGV4IDViMzVkZWJjOGZmLi42ZDQyOGQ2NWQ2NCAxMDA2NDQK
LS0tIGEvc3JjL2JhY2tlbmQvdXRpbHMvYWR0L3NlbGZ1bmNzLmMKKysrIGIvc3JjL2JhY2tl
bmQvdXRpbHMvYWR0L3NlbGZ1bmNzLmMKQEAgLTE1MCwxNCArMTUwLDEzIEBAIGdldF9pbmRl
eF9zdGF0c19ob29rX3R5cGUgZ2V0X2luZGV4X3N0YXRzX2hvb2sgPSBOVUxMOwogCiBzdGF0
aWMgZG91YmxlIGVxc2VsX2ludGVybmFsKFBHX0ZVTkNUSU9OX0FSR1MsIGJvb2wgbmVnYXRl
KTsKIHN0YXRpYyBkb3VibGUgZXFqb2luc2VsX2lubmVyKE9pZCBvcGZ1bmNvaWQsIE9pZCBj
b2xsYXRpb24sCi0JCQkJCQkJICBWYXJpYWJsZVN0YXREYXRhICp2YXJkYXRhMSwgVmFyaWFi
bGVTdGF0RGF0YSAqdmFyZGF0YTIsCiAJCQkJCQkJICBkb3VibGUgbmQxLCBkb3VibGUgbmQy
LAogCQkJCQkJCSAgYm9vbCBpc2RlZmF1bHQxLCBib29sIGlzZGVmYXVsdDIsCiAJCQkJCQkJ
ICBBdHRTdGF0c1Nsb3QgKnNzbG90MSwgQXR0U3RhdHNTbG90ICpzc2xvdDIsCiAJCQkJCQkJ
ICBGb3JtX3BnX3N0YXRpc3RpYyBzdGF0czEsIEZvcm1fcGdfc3RhdGlzdGljIHN0YXRzMiwK
IAkJCQkJCQkgIGJvb2wgaGF2ZV9tY3ZzMSwgYm9vbCBoYXZlX21jdnMyKTsKIHN0YXRpYyBk
b3VibGUgZXFqb2luc2VsX3NlbWkoT2lkIG9wZnVuY29pZCwgT2lkIGNvbGxhdGlvbiwKLQkJ
CQkJCQkgVmFyaWFibGVTdGF0RGF0YSAqdmFyZGF0YTEsIFZhcmlhYmxlU3RhdERhdGEgKnZh
cmRhdGEyLAorCQkJCQkJCSBWYXJpYWJsZVN0YXREYXRhICpyaHNfdmFyZGF0YSwKIAkJCQkJ
CQkgZG91YmxlIG5kMSwgZG91YmxlIG5kMiwKIAkJCQkJCQkgYm9vbCBpc2RlZmF1bHQxLCBi
b29sIGlzZGVmYXVsdDIsCiAJCQkJCQkJIEF0dFN0YXRzU2xvdCAqc3Nsb3QxLCBBdHRTdGF0
c1Nsb3QgKnNzbG90MiwKQEAgLTIzNDgsNyArMjM0Nyw2IEBAIGVxam9pbnNlbChQR19GVU5D
VElPTl9BUkdTKQogCiAJLyogV2UgbmVlZCB0byBjb21wdXRlIHRoZSBpbm5lci1qb2luIHNl
bGVjdGl2aXR5IGluIGFsbCBjYXNlcyAqLwogCXNlbGVjX2lubmVyID0gZXFqb2luc2VsX2lu
bmVyKG9wZnVuY29pZCwgY29sbGF0aW9uLAotCQkJCQkJCQkgICZ2YXJkYXRhMSwgJnZhcmRh
dGEyLAogCQkJCQkJCQkgIG5kMSwgbmQyLAogCQkJCQkJCQkgIGlzZGVmYXVsdDEsIGlzZGVm
YXVsdDIsCiAJCQkJCQkJCSAgJnNzbG90MSwgJnNzbG90MiwKQEAgLTIzNzUsNyArMjM3Myw3
IEBAIGVxam9pbnNlbChQR19GVU5DVElPTl9BUkdTKQogCiAJCQlpZiAoIWpvaW5faXNfcmV2
ZXJzZWQpCiAJCQkJc2VsZWMgPSBlcWpvaW5zZWxfc2VtaShvcGZ1bmNvaWQsIGNvbGxhdGlv
biwKLQkJCQkJCQkJCSAgICZ2YXJkYXRhMSwgJnZhcmRhdGEyLAorCQkJCQkJCQkJICAgJnZh
cmRhdGEyLAogCQkJCQkJCQkJICAgbmQxLCBuZDIsCiAJCQkJCQkJCQkgICBpc2RlZmF1bHQx
LCBpc2RlZmF1bHQyLAogCQkJCQkJCQkJICAgJnNzbG90MSwgJnNzbG90MiwKQEAgLTIzODgs
NyArMjM4Niw3IEBAIGVxam9pbnNlbChQR19GVU5DVElPTl9BUkdTKQogCQkJCU9pZAkJCWNv
bW1vcGZ1bmNvaWQgPSBPaWRJc1ZhbGlkKGNvbW1vcCkgPyBnZXRfb3Bjb2RlKGNvbW1vcCkg
OiBJbnZhbGlkT2lkOwogCiAJCQkJc2VsZWMgPSBlcWpvaW5zZWxfc2VtaShjb21tb3BmdW5j
b2lkLCBjb2xsYXRpb24sCi0JCQkJCQkJCQkgICAmdmFyZGF0YTIsICZ2YXJkYXRhMSwKKwkJ
CQkJCQkJCSAgICZ2YXJkYXRhMSwKIAkJCQkJCQkJCSAgIG5kMiwgbmQxLAogCQkJCQkJCQkJ
ICAgaXNkZWZhdWx0MiwgaXNkZWZhdWx0MSwKIAkJCQkJCQkJCSAgICZzc2xvdDIsICZzc2xv
dDEsCkBAIC0yNDM2LDcgKzI0MzQsNiBAQCBlcWpvaW5zZWwoUEdfRlVOQ1RJT05fQVJHUykK
ICAqLwogc3RhdGljIGRvdWJsZQogZXFqb2luc2VsX2lubmVyKE9pZCBvcGZ1bmNvaWQsIE9p
ZCBjb2xsYXRpb24sCi0JCQkJVmFyaWFibGVTdGF0RGF0YSAqdmFyZGF0YTEsIFZhcmlhYmxl
U3RhdERhdGEgKnZhcmRhdGEyLAogCQkJCWRvdWJsZSBuZDEsIGRvdWJsZSBuZDIsCiAJCQkJ
Ym9vbCBpc2RlZmF1bHQxLCBib29sIGlzZGVmYXVsdDIsCiAJCQkJQXR0U3RhdHNTbG90ICpz
c2xvdDEsIEF0dFN0YXRzU2xvdCAqc3Nsb3QyLApAQCAtMjYzMyw3ICsyNjMwLDcgQEAgZXFq
b2luc2VsX2lubmVyKE9pZCBvcGZ1bmNvaWQsIE9pZCBjb2xsYXRpb24sCiAgKi8KIHN0YXRp
YyBkb3VibGUKIGVxam9pbnNlbF9zZW1pKE9pZCBvcGZ1bmNvaWQsIE9pZCBjb2xsYXRpb24s
Ci0JCQkgICBWYXJpYWJsZVN0YXREYXRhICp2YXJkYXRhMSwgVmFyaWFibGVTdGF0RGF0YSAq
dmFyZGF0YTIsCisJCQkgICBWYXJpYWJsZVN0YXREYXRhICpyaHNfdmFyZGF0YSwKIAkJCSAg
IGRvdWJsZSBuZDEsIGRvdWJsZSBuZDIsCiAJCQkgICBib29sIGlzZGVmYXVsdDEsIGJvb2wg
aXNkZWZhdWx0MiwKIAkJCSAgIEF0dFN0YXRzU2xvdCAqc3Nsb3QxLCBBdHRTdGF0c1Nsb3Qg
KnNzbG90MiwKQEAgLTI2NjIsMTEgKzI2NTksMTEgQEAgZXFqb2luc2VsX3NlbWkoT2lkIG9w
ZnVuY29pZCwgT2lkIGNvbGxhdGlvbiwKIAkgKiBncmVhdCwgbWF5YmUsIGJ1dCBpdCBkaWRu
J3QgY29tZSBvdXQgb2Ygbm93aGVyZSBlaXRoZXIuICBUaGlzIGlzIG1vc3QKIAkgKiBoZWxw
ZnVsIHdoZW4gdGhlIGlubmVyIHJlbGF0aW9uIGlzIGVtcHR5IGFuZCBjb25zZXF1ZW50bHkg
aGFzIG5vIHN0YXRzLgogCSAqLwotCWlmICh2YXJkYXRhMi0+cmVsKQorCWlmIChyaHNfdmFy
ZGF0YS0+cmVsKQogCXsKLQkJaWYgKG5kMiA+PSB2YXJkYXRhMi0+cmVsLT5yb3dzKQorCQlp
ZiAobmQyID49IHJoc192YXJkYXRhLT5yZWwtPnJvd3MpCiAJCXsKLQkJCW5kMiA9IHZhcmRh
dGEyLT5yZWwtPnJvd3M7CisJCQluZDIgPSByaHNfdmFyZGF0YS0+cmVsLT5yb3dzOwogCQkJ
aXNkZWZhdWx0MiA9IGZhbHNlOwogCQl9CiAJfQotLSAKMi4zNC4xCgo=

--------------Nqo9DALGO0un850G0z4IhyCu--