From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
cc: PostgreSQL-development <pgsql-hackers@postgresql.org>
Subject: Re: btree_gin and btree_gist for enums
In-reply-to: <3b969375-dcb5-aa73-b8e9-cacd5c8d60ef@2ndQuadrant.com>
References: <56EA8A71.8060107@dunslane.net> <56EACB8C.1040406@dunslane.net>
 <CAE2gYzyB=vyquQ2hEWA9UwNb=zYAKFsNO5vBtbfkV2=PzoPZjg@mail.gmail.com>
 <03f7d748-0f89-e8e1-094d-f04e1026b93d@dunslane.net>
 <27220.1478360811@sss.pgh.pa.us>
 <e60f50c4-899a-d48a-6cb4-4611acf2f73c@dunslane.net>
 <31412.1478366009@sss.pgh.pa.us>
 <60fb708c-8b3c-1337-934e-c1bfe2afc539@dunslane.net>
 <db2b70a4-78d7-294a-a315-8e7f506c5978@2ndQuadrant.com>
 <8258.1487886101@sss.pgh.pa.us>
 <3b969375-dcb5-aa73-b8e9-cacd5c8d60ef@2ndQuadrant.com>
Comments: In-reply-to Andrew Dunstan <andrew.dunstan@2ndquadrant.com>
	message dated "Thu, 23 Feb 2017 16:45:00 -0500"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <25088.1487899876.0@sss.pgh.pa.us>
Date: Thu, 23 Feb 2017 20:34:27 -0500
Message-ID: <25226.1487900067@sss.pgh.pa.us>
Precedence: bulk
Sender: pgsql-hackers-owner@postgresql.org

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

Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> On 02/23/2017 04:41 PM, Tom Lane wrote:
>> BTW, while I'm looking at this ... isn't gin_enum_cmp broken on its face?

> Yes, that's what I'm trying to fix.

I'd forgotten where this thread started.  For a minute there I thought
we had a live bug, rather than a deficiency in code-under-development.

After thinking about that, I believe that enum_cmp_internal is a rather
considerable hazard.  It might not be our only function that requires
fcinfo->flinfo cache space just some of the time not all the time, but
I don't recall anyplace else that could so easily undergo a reasonable
amount of testing without *ever* reaching the case where it requires
that cache space.  So I'm now worried that it wouldn't be too hard for
such a bug to get past us.

I think we should address that by adding a non-bypassable Assert that
the caller did provide flinfo, as in the attached.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="assert-that-enum-comparator-can-use-cache.patch";
	charset="us-ascii"
Content-ID: <25088.1487899876.2@sss.pgh.pa.us>
Content-Description: assert-that-enum-comparator-can-use-cache.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/backend/utils/adt/enum.c b/src/backend/utils/adt/enum.c
index 8110ee2..b1d2a6f 100644
*** a/src/backend/utils/adt/enum.c
--- b/src/backend/utils/adt/enum.c
*************** enum_cmp_internal(Oid arg1, Oid arg2, Fu
*** 263,268 ****
--- 263,277 ----
  {
  	TypeCacheEntry *tcache;
=20=20
+ 	/*
+ 	 * We don't need the typcache except in the hopefully-uncommon case that
+ 	 * one or both Oids are odd.  This means that cursory testing of code th=
at
+ 	 * fails to pass flinfo to an enum comparison function might not disclose
+ 	 * the oversight.  To make such errors more obvious, Assert that we have=
 a
+ 	 * place to cache even when we take a fast-path exit.
+ 	 */
+ 	Assert(fcinfo->flinfo !=3D NULL);
+=20
  	/* Equal OIDs are equal no matter what */
  	if (arg1 =3D=3D arg2)
  		return 0;
*************** enum_cmp(PG_FUNCTION_ARGS)
*** 381,392 ****
  	Oid			a =3D PG_GETARG_OID(0);
  	Oid			b =3D PG_GETARG_OID(1);
=20=20
! 	if (a =3D=3D b)
! 		PG_RETURN_INT32(0);
! 	else if (enum_cmp_internal(a, b, fcinfo) > 0)
! 		PG_RETURN_INT32(1);
! 	else
! 		PG_RETURN_INT32(-1);
  }
=20=20
  /* Enum programming support functions */
--- 390,396 ----
  	Oid			a =3D PG_GETARG_OID(0);
  	Oid			b =3D PG_GETARG_OID(1);
=20=20
! 	PG_RETURN_INT32(enum_cmp_internal(a, b, fcinfo));
  }
=20=20
  /* Enum programming support functions */

------- =_aaaaaaaaaa0
Content-Type: text/plain
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

------- =_aaaaaaaaaa0--