Re: Binary search in ScalarArrayOpExpr for OR'd constant arrays

From: David Rowley <dgrowleyml(at)gmail(dot)com>
To: James Coleman <jtc331(at)gmail(dot)com>
Cc: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Tomas Vondra <tomas(dot)vondra(at)enterprisedb(dot)com>, Michael Paquier <michael(at)paquier(dot)xyz>, Andrew Dunstan <andrew(at)dunslane(dot)net>, Peter Geoghegan <pg(at)bowt(dot)ie>, Heikki Linnakangas <hlinnaka(at)iki(dot)fi>, Tomas Vondra <tomas(dot)vondra(at)2ndquadrant(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Binary search in ScalarArrayOpExpr for OR'd constant arrays
Date: 2021-04-24 10:25:25
Message-ID: CAApHDvp-ecKwa4sBmWXhYkg2b0Gdi9_5stPFffer6261sog+dA@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Wed, 14 Apr 2021 at 05:40, James Coleman <jtc331(at)gmail(dot)com> wrote:
> ...and here's a draft patch. I can take this to a new thread if you'd
> prefer; the one here already got committed, on the other hand this is
> pretty strongly linked to this discussion, so I figured it made sense
> to post it here.

I only glanced at this when you sent it and I was confused about how
it works. The patch didn't look like how I imagined it should and I
couldn't see how the executor part worked without any changes.

Anyway, I decided to clear up my confusion tonight and apply the patch
to figure all this out... unfortunately, I see why I was confused
now. It actually does not work at all :-(

You're still passing the <> operator to get_op_hash_functions(), which
of course is not hashable, so we just never do hashing for NOT IN.

All your tests pass just fine because the standard non-hashed code path is used.

My idea was that you'd not add any fields to ScalarArrayOpExpr and for
soaps with useOr == false, check if the negator of the operator is
hashable. If so set the opfuncid to the negator operator's function.

I'm a bit undecided if it's safe to set the opfuncid to the negator
function. If anything were to set that again based on the opno then
it would likely set it to the wrong thing. We can't go changing the
opno either because EXPLAIN would display the wrong thing.

Anyway, I've attached what I ended up with after spending a few hours
looking at this.

I pretty much used all your tests as is with the exception of removing
one that looked duplicated.

David

Attachment Content-Type Size
v2-0001-Speedup-NOT-IN-with-a-set-of-Consts.patch application/octet-stream 12.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Andrew Dunstan 2021-04-24 13:49:07 Re: multi-install PostgresNode fails with older postgres versions
Previous Message Amul Sul 2021-04-24 06:16:05 Re: fix a comment