From: Tom Lane <tgl@sss.pgh.pa.us>
To: David Rowley <dgrowleyml@gmail.com>
cc: Sergei Glukhov <s.glukhov@postgrespro.ru>,
        pgsql-hackers@lists.postgresql.org
Subject: Re: Partial hash index is not used for implied qual.
In-reply-to: 
 <CAApHDvpVwXQgiatD_FM_F3fYKU-j1kqPeTQn-xeRptc2nyGRVw@mail.gmail.com>
References: <e200bf38-6b45-446a-83fd-48617211feff@postgrespro.ru>
 <CAApHDvpVazvK7jAjpMWW5FYR4RQZ2NT8HidS03un8T+Ha8ZQDw@mail.gmail.com>
 <1106189.1764033798@sss.pgh.pa.us> <1112416.1764036072@sss.pgh.pa.us>
 <CAApHDvpVwXQgiatD_FM_F3fYKU-j1kqPeTQn-xeRptc2nyGRVw@mail.gmail.com>
Comments: In-reply-to David Rowley <dgrowleyml@gmail.com>
	message dated "Wed, 26 Nov 2025 01:36:52 +1300"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <1615191.1764181481.0@sss.pgh.pa.us>
Date: Wed, 26 Nov 2025 13:25:57 -0500
Message-ID: <1615744.1764181557@sss.pgh.pa.us>
Archived-At: 
 <https://www.postgresql.org/message-id/1615744.1764181557%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <1615191.1764181481.1@sss.pgh.pa.us>

David Rowley <dgrowleyml@gmail.com> writes:
> On Tue, 25 Nov 2025 at 15:01, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Actually, after thinking a bit longer, it'd be better to do something
>> like the attached so that we don't keep redundant quals unless they'd
>> *all* be excluded.

> I think your 1st patch was more along the correct lines. With the 2nd
> one, I think you're maybe assuming that the non-empty newrestrictinfo
> must contain an indexable qual, but the list we're working with at
> that point is the rel's baserestrictinfo, which could contain a bunch
> of stuff that'll never match to the index. If you continue to remove
> the implied qual when there's a non-indexable base qual in the list,
> then we'll still have the same issue that Sergei is trying to fix.

Right, as Sergei also noted.  We should just do it as attached.

I checked the costing calculations and it's basically that
genericcostestimate doesn't understand about hash buckets.
For the partial index, it correctly estimates that we'll visit
all 10 of the tuples in the index, so it supposes that that
means we'll visit all of the index's pages.  For the non-partial
index, it correctly estimates that we'll visit 10 of the 1000
tuples in the index, so it supposes that that means we'll visit
1/100th of the index's pages (rounded up to 1).  This error is
compounded by the toy example, which only has 6 pages in either index
(the minimum size of a hash index, I think).  There may or may not be
anything we can usefully do to improve that situation ... and for that
matter, it's not clear that preferring the partial index would really
be a win.  As constructed, this test case has only one hash value in
the partial index, which I think is not exactly a case where you want
a hash index.  I tried scaling up the table size, and got a badly
bloated partial index (about half as big as the non-partial one),
which seems to indicate that the code is vainly splitting and
re-splitting trying to divide that one huge bucket.

So I'm inclined to apply the attached and just call it good.

Should we back-patch?  I'm unsure.  Clearly it's a bug that we
cannot generate an indexscan plan in this case, but we've learned
that changing plans in released branches is often not wanted.
And given the lack of field complaints, nobody is using the case
anyway.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff; name="v2-fix-partial-hash-index-scan.patch";
	charset="us-ascii"
Content-ID: <1615191.1764181481.2@sss.pgh.pa.us>
Content-Description: v2-fix-partial-hash-index-scan.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/optimizer/path/indxpath.c b/src/backend/optimizer=
/path/indxpath.c
index c62e3f87724..2654c59c4c6 100644
--- a/src/backend/optimizer/path/indxpath.c
+++ b/src/backend/optimizer/path/indxpath.c
@@ -4051,6 +4051,16 @@ check_index_predicates(PlannerInfo *root, RelOptInf=
o *rel)
 		if (is_target_rel)
 			continue;
 =

+		/*
+		 * If index is !amoptionalkey, also leave indrestrictinfo as set
+		 * above.  Otherwise we risk removing all quals for the first index
+		 * key and then not being able to generate an indexscan at all.  It
+		 * would be better to be more selective, but we've not yet identified
+		 * which if any of the quals match the first index key.
+		 */
+		if (!index->amoptionalkey)
+			continue;
+
 		/* Else compute indrestrictinfo as the non-implied quals */
 		index->indrestrictinfo =3D NIL;
 		foreach(lcr, rel->baserestrictinfo)

------- =_aaaaaaaaaa0--