Feedback-ID: ia2694551:Fastmail
Date: Wed, 12 Feb 2025 11:55:43 +0100
From: =?utf-8?Q?=C3=81lvaro?= Herrera <alvherre@alvh.no-ip.org>
To: Tomas Vondra <tomas@vondra.me>
Cc: Tobias Wendorff <tobias.wendorff@tu-dortmund.de>,
	pgsql-bugs@lists.postgresql.org
Subject: Re: BRIN index creation on geometry column causes crash
Message-ID: <202502121055.ufgofvji2kuv@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="3wzbj3zscr6r7fvh"
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <9b346570-675c-47a5-ab2b-131bcc80988b@vondra.me>
Archived-At: 
 <https://www.postgresql.org/message-id/202502121055.ufgofvji2kuv%40alvherre.pgsql>
Precedence: bulk


--3wzbj3zscr6r7fvh
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit

On 2025-Jan-03, Tomas Vondra wrote:

> Reproduced, but I belive this is actually a long-standing PostGIS bug.
> The exact place where it crashes is here:
> 
> /* Finally, merge B to A. */
> finfo = inclusion_get_procinfo(bdesc, attno, PROCNUM_MERGE);
> Assert(finfo != NULL);
> result = FunctionCall2Coll(finfo, colloid, ...);
> 
> With asserts, it fails on the assert, i.e. finfo is NULL. This means the
> opfamily is missing the "MERGE" support procedure, which is however
> required (from the very beginning of BRIN in 9.5).

Hmm, I completely agree that this is a PostGIS bug, but at the same time
I think it's wrong for Postgres not to verify that the support function
exists, so that we can throw an error instead of crashing.  I think the
most expedient way to implement this is to add a "missing_ok" argument
to the various "foo_get_procinfo" functions, and make to pass it true
only in cases where the usage of the return value already admit that it
could be a null pointer (meaning it's really an optional support proc).

This should avoid further crashes with invalidly defined opclasses; see
attached POC patch.  The user is still in a bad place, because for
instance if it's autovacuum that tries to do the range merging, then
vacuuming will fail but people may not notice for months unless they're
paying careful attention to the server logs.  Crashing is good (tm)
because now a bunch of people are aware that the opclass is broken.

My next question is why didn't brinvalidate detect this problem.  Is it
just that nobody ran 'amvalidate' on the opclass, or is it that
brinvalidate does not know that it needs to require the merge proc?  But
I'll leave that for later.


Now, in writing this patch I noticed that both brin_bloom.c and
brin_minmax_multi.c seem to have cargo-culted the idea that support
procs can be optional; both of them have a single support proc
(PROCNUM_HASH and PROCNUM_DISTANCE, respectively) which is not optional;
so the "extra_proc_missing" stuff seems unnecessary for them.  And
therefore the new missing_ok support I introduce in this patch would
also be unnecessary.  As far as I can tell, there are no ABI concerns
about removing opaque->extra_proc_missing[] from both these opclass
scaffolds (and reintroducing it later, if we determine that we do need
optional support procs in those scaffolds after all.)

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Saca el libro que tu religión considere como el indicado para encontrar la
oración que traiga paz a tu alma. Luego rebootea el computador
y ve si funciona" (Carlos Duclós)

--3wzbj3zscr6r7fvh
Content-Type: text/x-diff; charset=utf-8
Content-Disposition: attachment;
	filename="v1-0001-BRIN-be-stricter-about-required-support-procs.patch"