Do you use PostGIS BRIN?

Tue Jan 7 08:07:25 PST 2025

On 1/6/25 23:24, Regina Obe wrote:
>> On 1/5/25 08:53, Regina Obe wrote:
>>> We are thinking of removing BRIN support in PostGIS because it has a serious
>>> bug as discussed here.
>>>
>>> https://lists.osgeo.org/pipermail/postgis-devel/2025-January/030457.html
>>>
>>> and here - https://trac.osgeo.org/postgis/ticket/5564
>>>
>>> that currently causes crashes during Index building.
>>>
>>> The two options on the table are:
>>>
>>> 1) Fix the issue
>>> 2) Remove BRIN Support
>>>
>>>
>>> I'm leaning toward the removal of BRIN support because I see no point
>>> carrying the technical baggage of continuously supporting BRIN and testing
>>> it 
>>> if very very few people are using it.  Yes this will be a breaking change
>>> for those that use it.
>>>
> 
>> I'm not really involved in PostGIS and I'm not sure how widely used the
>> BRIN opclasses are, but I'd be willing to help with fixing/testing them.
>> I find BRIN indexes to be quite neat, it'd be somewhat sad if one of the
>> main projects built on Postgres ditched them.
> 
> From discussions with some folks, it is more widely used than I had expected.
> But the two uses I have heard from so far are one-offs for having a lightweight and relatively fast to build
> Index for openstreetmap data and the like, which as much as I can guess is then thrown away after data is completely loaded.
> 

Understood. Interesting use of BRIN to build indexes quickly, until the
regular indexes get built. It probably does not make sense to keep the
BRIN indexes after that - the other indexes are likely to be faster, so
it'd just allow the planner to make mistakes.

>> I haven't tried but my impression is the fix wouldn't be that hard. Ofc,
>> that doesn't take away the long-term maintenance cost, but AFAIK this
>> shouldn't break very often either.
>> regards
> 
>> -- 
>> Tomas Vondra
> 
> The annoyance I have is several people have said it should be easy to fix, but no one has stepped up to the plate
> to put their time where their mouth is.  But if you are willing to put in the time, that pushes us closer in the  "Keeping" direction.
> 

Yeah. I spent some time today looking at this, and it's indeed a bit
more complicated than I assumed. There's a couple reasons ...

1) The opclass (I only looked at brin_geometry_inclusion_ops_2d) has
opcintype != opckeytype, but the BRIN code does not actually expect (or
allow) that. I guess this might be why the opclass overrides the
ADD_VALUE function, because the built-in function simply uses the first
value as the union, without converting it to box2df.

This means we can't get rid of the custom add_value, but AFAICS the
_merge is just a simplified variant of it, as it only deals with the
box2df summaries.

2) I don't think there's a way to add a procedure to existing opclass,
i.e. there's nothing like ALTER OPERATOR CLASS ... FUNCTION ... :-(

So it would need to be a completely new opclass.

> The other issue I have is with the upgrade issues we'll run into.  Based on my understanding, we'd need to modify the opclasses
> and introduce more functions.  This is feels kind of dirty back-porting such a change - adding functions to "Stable" versions of PostGIS.
> I think we've done that once before.
> 
> My idea of removing it wasn't to remove it so much as neuter it for the stable versions.
> By that I mean changing one of the backend functions, so it just errors out on index building etc so we don't need to touch the SQL API at all
> And anyone running with just the libs that forgot to do the  SELECT postgis_extensions_upgrade();  would be safe from accidentally crashing.
> 

Yes. I think the main challenge is the case when people installed the
new library, but are running with the old definitions. And you're right
the best solution would be to "neuter" the "old" functions by adding
something like elog(ERROR) into them.

But as I wrote above, I'm not aware of a way to modify an opclass
(there's ALTER OPERATOR CLASS, but it only allows changing name, owner
and schema).

So the existing opclass would need to be neutered, and a completely new
opclass would need to be added.

> I haven't walked thru if that is even doable, but my thought was at
least we wouldn't be introducing new functions in stable versions of PostGIS
> We'd just be changing the behavior to not do harmful things.
> 
> Then later on we could decide to fix it, only fix it for PostGIS
> 3.6, or just remove the entrails of BRIN for 3.6 and above.

I think adding new functions is not a breaking change - there's the risk
people might not have updated the definitions, but in the worst case the
those functions would not be called. It's the removal of functions that
breaks things.

Attached is a patch fixing the brin_geometry_inclusion_ops_2d opclass
(at least I believe so). The 3d/4d opclasses would need a similar fix,
derived from gidx_brin_inclusion_add_value.

This doesn't make amvalidate 100% happy though, it still complains about
missing functions, but I believe it just gets confused by the cross-type
operators.

regards

-- 
Tomas Vondra
-------------- next part --------------
A non-text attachment was scrubbed...
Name: postgis-brin-fix.patch
Type: text/x-patch
Size: 2345 bytes
Desc: not available
URL: <http://lists.osgeo.org/pipermail/postgis-devel/attachments/20250107/47059bdd/attachment.bin>