Date: Tue, 7 Jan 2020 15:39:07 +0900
From: Michael Paquier <michael@paquier.xyz>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: Andrey Borodin <x4mmm@yandex-team.ru>,
	Tomas Vondra <tomas.vondra@2ndquadrant.com>,
	Adam Scott <adam.c.scott@gmail.com>,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BUG #16122: segfault pg_detoast_datum (datum=0x0) at fmgr.c:1833
 numrange query
Message-ID: <20200107063907.GF2386@paquier.xyz>
References: <16122-eb35bc248c806c15@postgresql.org>
 <20191118120312.cblcswef6vuqbijq@development>
 <CA+s62-Mnj-x8_ZXh7tBVU3WJ3cKUsUL2HAp2YaJSXQPw1iypFw@mail.gmail.com>
 <20191119014827.GC1614@paquier.xyz>
 <20191119103813.xnsloisoxkwrxoim@development>
 <20191119114056.GA516103@paquier.xyz>
 <20191210053815.GI72921@paquier.xyz>
 <837E8CEE-FFF3-497C-B7C2-37BB42FAF2C5@yandex-team.ru>
 <DE43C854-F3BD-4247-93F3-2774B643445C@yandex-team.ru>
 <5794.1578342514@sss.pgh.pa.us>
MIME-Version: 1.0
Content-Type: multipart/signed; micalg=pgp-sha512;
	protocol="application/pgp-signature"; boundary="L1c6L/cjZjI9d0Eq"
Content-Disposition: inline
In-Reply-To: <5794.1578342514@sss.pgh.pa.us>
Precedence: bulk


--L1c6L/cjZjI9d0Eq
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
Content-Transfer-Encoding: quoted-printable

On Mon, Jan 06, 2020 at 03:28:34PM -0500, Tom Lane wrote:
> I believe that Michael's draft fix is actually about right, because
> we don't want to treat the key space above the histogram upper bound
> as being an actual bin: we should assume that there are no values there.
> So just starting the loop at the last real bin is sufficient.  However,
> I'd prefer to handle the two edge cases (i.e., probe value below histogram
> lower bound or above histogram upper bound) explicitly, because that makes
> it more apparent what's going on.

Actually, I suspect that the original error comes from a copy-pasto
=66rom calc_hist_selectivity_contains to calc_hist_selectivity_contained
when the code has been originally written.

> There are a couple of other ways in which this code seems insufficiently
> paranoid to me:
>=20
> * It seems worth spending a couple lines of code at the beginning to
> verify that the histogram has at least one bin, as it already did for
> the range-length histogram.  Otherwise we've got various divide-by-zero
> hazards here, along with a need for extra tests in the looping functions.

Fine by me.

> * I think we'd better guard against NaNs coming back from the range
> subdiff function, as well as the possibility of getting a NaN from
> the division in get_position (compare [1]).

Good idea.

> * I am not quite totally sure about this part, but it seems to me
> that calc_hist_selectivity_contains probably ought to handle the
> range-bound cases the same as calc_hist_selectivity_contained,
> even though only the latter is trying to access an unsafe array
> index.

Both could actually be merged, no?  The assumptions behind the
upper/lower bound lookups are actually just mirrors of each other
based on which side of the operators <@ or @> the code looks at.
Perhaps the current code is cleaner as done currently anyway..

> That all leads me to the attached draft patch.  It lacks a test
> case --- I wonder if we can find one that crashes more portably
> than Michael's?  And more eyeballs on calc_hist_selectivity_contains
> would be good too.

It would be nice to keep the test case as it crashes immediately on
own Debian laptop.  Anyway, if we add a test, we really need
something that pokes at the bin with the histogram with the upper
bound.  Just something like the attached on HEAD proves that we have
zero coverage for it:=20
--- a/src/backend/utils/adt/rangetypes_selfuncs.c
+++ b/src/backend/utils/adt/rangetypes_selfuncs.c
@@ -1050,6 +1050,7 @@ calc_hist_selectivity_contained(TypeCacheEntry *typca=
che,
    double      dist;
    double      length_hist_frac;
    bool        final_bin =3D false;
+   Assert(i =3D=3D upper_index);
So that's not good from the start..

-   if (bin_width <=3D 0.0)
+   if (isnan(bin_width) || bin_width <=3D 0.0)
 	return 0.5;			/* zero width bin */
This comment is incorrect now?

+   if (isnan(position))
+       return 0.5;			/* punt if we have any NaNs or Infs */
+
This comment is incorrect as well, no?  In this case both histograms
have finite bounds, and you just check if that's a number.
--
Michael

--L1c6L/cjZjI9d0Eq
Content-Type: application/pgp-signature; name="signature.asc"

-----BEGIN PGP SIGNATURE-----

iQIzBAABCgAdFiEEG72nH6vTowiyblFKnvQgOdbyQH0FAl4UJ4sACgkQnvQgOdby
QH3LAg//W0gaRhontv3+ZY5cBvu8AhQfDYlSkzQ/wVhBBy7SFNb+63VbM8lQAPVW
BKT9mFX0kW061K13OkPwmjx+G68pbxdPf6lbkZuPiX7mShtZtaCRDUROU8f7giRa
Mewidqi4Ox8te3DKmSA///U0xdySxFyzDyuFn0VVKnR63sDS6bC4+8Z012Tgn3HE
IybT3YfvIyqNq2bfKARML/j1Kr4v67TRXYnBNwrVsa10qTKepdTw4+VGIMgV9p2q
INf+D8Bq1V3VRVMg6h5lB01nubq9cjgTVpAFF0nrcfhZpQMnHJlA6EKQFLP6M4YD
jg7hvyiStt1yg1bDV4u+8lvhSuR6Yz3b35Fla3Vs+pSb1dqNyLqqbMYnx0P8+TEj
6L9edzQ3zyrUZmZ9ppHuSXMMELkoOUyunD4OTWG2XBY8kRaw4u61h8QnsZZkW734
/5JxnujGTMFjZ6WwTAm5JMoSIpxtBPf4KBELKTP5eg98V8jvFQcqZwDN0pW0RKAV
rIYjPrJqYsUgN97TlYXABDThUAMVm/CW/eKrLYWuJk/PIyxqQvpuiBtc2t0vO+Ml
8z9ksEK0ybzZx0Ba/xTHpB8uTqWRf67mBXJNLv1ILm3VAuy7hbFUEfoNAFgodm6T
1jFo9cnKerzHmjkyWUQupoHaurlxHK0JDWRqq8xHdFXlTdbDqCU=
=Vgbl
-----END PGP SIGNATURE-----

--L1c6L/cjZjI9d0Eq--