On Thu, Jul 25, 2024 at 12:07 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I took a brief look at this. I think the basic idea is sound,
> but I have a couple of nits:
Thank you for reviewing this patch!
> * It's not entirely obvious that the checks preceding these additions
> are sufficient to ensure that the clauses are OpExprs. They probably
> are, since the clauses are marked hashable or mergeable, but that test
> is mighty far away. More to the point, if they ever weren't OpExprs
> the result would likely be to pass a bogus OID to get_commutator and
> thus silently fail, allowing the problem to go undetected for a long
> time. I'd suggest using castNode() rather than a hard-wired
> assumption that the clause is an OpExpr.
Good point. I've modified the code to use castNode(), and added
comment accordingly.
> * Do we really need to construct a whole new set of bogus operators
> and opclasses to test this? As you noted, the regression tests
> already set up an incomplete opclass for other purposes. Why can't
> we extend that test, to reduce the amount of cycles wasted forevermore
> on this rather trivial point?
>
> (I'm actually wondering whether we really need to memorialize this
> with a regression test case at all. But I'd settle for minimizing
> the amount of test cycles added.)
At first I planned to use the alias type 'int8alias1' created in
equivclass.sql for this test. However, I found that this type is not
created yet when running join.sql. Perhaps it's more convenient to
place this test in equivclass.sql to leverage the bogus operators
created there, but the test does not seem to be related to 'broken'
ECs. I'm not sure if equivclass.sql is the appropriate place.
Do you think it works if we place this test in equivclass.sql and
write a comment explaining why it's there, like attached? Now I’m
also starting to wonder if this change actually warrants such a test.
BTW, do you think this patch is worth backpatching?
Thanks
Richard