Thread: Re: WIP: BRIN multi-range indexes

On 3/17/21 7:59 PM, John Naylor wrote:
> On Thu, Mar 11, 2021 at 12:26 PM Tomas Vondra
> <tomas.vondra@enterprisedb.com <mailto:tomas.vondra@enterprisedb.com>>
> wrote:
>>
>> Hi,
>>
>> Here is an updated version of the patch series.
>>
>> It fixes the assert failure (just remove the multiplication from it) and
>> adds a simple regression test that exercises this.
>>
>> Based on the discussion so far, I've decided to keep just the new
>> signature of the consistent function. That's a bit simpler than having
>> to support both 3 and 4 parameters, and it would not deal with the NULL
>> changes anyway (mostly harmless code duplication, but still). I've also
>> realized the API documentation in SGML needs updating.
>>
>> At this point, I think 0001-0006 parts are mostly committable.
> 
> I think so. I've marked it RFC for this six.
> 
>> As for the remaining two parts, the one dealing with correlation may not
>> be strictly necessary, but not having it (or something like it) may
>> result in not picking the BRIN index in some cases.
>>
>> But maybe it's not a major problem. I tried the example from [1] but it
>> no longer triggers the issue for me - I'm not entirely sure why, but the
>> costing changed for some reason. It used to look like this:
> 
>> ...
> 
>> The index scan cost is about the same, but the heap scan is about half
>> the cost. The row estimates are a bit different, perhaps that's related.
>> The seqscan cost (169248) and duration (~500ms) is still about the same,
>> but so now we still pick the bitmap heap scan.
> 
> With only 0001-0006, I get a parallel bitmap scan in both cases:
> 
>                                                             QUERY PLAN
>
-----------------------------------------------------------------------------------------------------------------------------------
>  Gather  (cost=6542.42..52779.35 rows=10 width=4) (actual
> time=3.283..22.308 rows=10 loops=1)
>    Workers Planned: 2
>    Workers Launched: 2
>    ->  Parallel Bitmap Heap Scan on t0  (cost=5542.42..51778.35 rows=4
> width=4) (actual time=3.434..17.257 rows=3 loops=3)
>          Recheck Cond: (a = 10000)
>          Rows Removed by Index Recheck: 83165
>          Heap Blocks: lossy=421
>          ->  Bitmap Index Scan on t0_a_idx  (cost=0.00..5542.42
> rows=381682 width=0) (actual time=2.996..2.996 rows=11040 loops=1)
>                Index Cond: (a = 10000)
>  Planning Time: 0.088 ms
>  Execution Time: 22.341 ms
> (11 rows)
> 
>> Not sure we can rely on
>> this, though. Would be quite sad if we had new improved opclasses but
>> the planner often decided not to use them.
> 
> Yeah, I'm not sure what to do here. It might be okay to leave it out now
> and study it further as a PG14 open item or PG15 improvement.
> 

Yeah, that's definitely an option.

>> I had an idea of tweaking choose_bitmap_and to consider both the cost
>> and selectivity (similarly to how add_path considers statup/total cost),
>> and that did indeed resolve this particular case. This is what the 0008
>> part does.
>>
>> But it may also have negative consequence, as demonstrated by the
>> reproducer2.sql script. So maybe the logic would need to be more
>> complicated. Or maybe there's no reliable solution, considering how
>> tricky/unreliable BRIN estimates are.
> 
> Ok, so with 0008 in reproducer2, it chooses the more selective path,
> even though it has a higher total cost:
> 
> 0001-0007:
> 
>                                                      QUERY PLAN
>
---------------------------------------------------------------------------------------------------------------------
>  Bitmap Heap Scan on t2  (cost=29.03..24032.28 rows=1 width=8) (actual
> time=0.498..1.755 rows=1 loops=1)
>    Recheck Cond: (a = 1000)
>    Rows Removed by Index Recheck: 7167
>    Heap Blocks: lossy=128
>    ->  Bitmap Index Scan on idx_2  (cost=0.00..29.03 rows=7163 width=0)
> (actual time=0.278..0.278 rows=1280 loops=1)
>          Index Cond: (a = 1000)
>  Planning Time: 0.148 ms
>  Execution Time: 1.774 ms
> (8 rows)
> 
> DROP INDEX
>                                                     QUERY PLAN
> -------------------------------------------------------------------------------------------------------------------
>  Bitmap Heap Scan on t2  (cost=656.00..1531.00 rows=1 width=8) (actual
> time=9.695..9.708 rows=1 loops=1)
>    Recheck Cond: (a = 1000)
>    Rows Removed by Index Recheck: 223
>    Heap Blocks: lossy=4
>    ->  Bitmap Index Scan on idx_1  (cost=0.00..656.00 rows=224 width=0)
> (actual time=9.675..9.675 rows=40 loops=1)
>          Index Cond: (a = 1000)
>  Planning Time: 0.110 ms
>  Execution Time: 9.727 ms
> (8 rows)
> 
> and with 0008:
> 
>                                                     QUERY PLAN
> -------------------------------------------------------------------------------------------------------------------
>  Bitmap Heap Scan on t2  (cost=656.00..1531.00 rows=1 width=8) (actual
> time=8.540..8.577 rows=1 loops=1)
>    Recheck Cond: (a = 1000)
>    Rows Removed by Index Recheck: 223
>    Heap Blocks: lossy=4
>    ->  Bitmap Index Scan on idx_1  (cost=0.00..656.00 rows=224 width=0)
> (actual time=8.507..8.508 rows=40 loops=1)
>          Index Cond: (a = 1000)
>  Planning Time: 0.175 ms
>  Execution Time: 8.601 ms
> (8 rows)
> 
> DROP INDEX
>                                                     QUERY PLAN
> -------------------------------------------------------------------------------------------------------------------
>  Bitmap Heap Scan on t2  (cost=656.00..1531.00 rows=1 width=8) (actual
> time=9.712..9.725 rows=1 loops=1)
>    Recheck Cond: (a = 1000)
>    Rows Removed by Index Recheck: 223
>    Heap Blocks: lossy=4
>    ->  Bitmap Index Scan on idx_1  (cost=0.00..656.00 rows=224 width=0)
> (actual time=9.691..9.692 rows=40 loops=1)
>          Index Cond: (a = 1000)
>  Planning Time: 0.104 ms
>  Execution Time: 9.746 ms
> (8 rows)
> 
> 
>> That being said, I don't think this is something we need to solve here,
>> and it may not actually be an issue at all. For this to happen there
>> need to be a terrible index on the same attribute (like the minmax index
>> in the example above). But why keeping such index anyway? Dropping it
>> would make the issue go away. If we have two indexes that both perform
>> reasonably (say, bloom and minmax-multi), the consequences are not that
>> bad. so this is interesting, but probably fine.
> 
> Yeah, I suspect this is unlikely to be a problem in practice.
> 
> --
> I've run a similar test based on an earlier example from some months ago
> (attached).
> 

Ummm, no attachment ;-)

> 0001-0006:
> 
> At first regular BRIN is faster, and it will choose it if available:
> 
>                                                                        
>   QUERY PLAN
>
--------------------------------------------------------------------------------------------------------------------------------------------------------------
>  Bitmap Heap Scan on iot  (cost=404.79..233352.86 rows=1252175 width=57)
> (actual time=2.115..346.351 rows=1252800 loops=1)
>    Recheck Cond: ((create_dt >= '2020-02-01 00:00:00-04'::timestamp with
> time zone) AND (create_dt < '2020-03-01 00:00:00-04'::timestamp with
> time zone))
>    Rows Removed by Index Recheck: 15936
>    Heap Blocks: lossy=15104
>    ->  Bitmap Index Scan on cd_multi  (cost=0.00..91.74 rows=1256702
> width=0) (actual time=0.972..0.972 rows=151040 loops=1)
>          Index Cond: ((create_dt >= '2020-02-01 00:00:00-04'::timestamp
> with time zone) AND (create_dt < '2020-03-01 00:00:00-04'::timestamp
> with time zone))
>  Planning Time: 0.208 ms
>  Execution Time: 412.549 ms
> (8 rows)
> 
>                                                                        
>   QUERY PLAN
>
--------------------------------------------------------------------------------------------------------------------------------------------------------------
>  Bitmap Heap Scan on iot  (cost=341.99..233335.81 rows=1256871 width=57)
> (actual time=1.244..170.962 rows=1252800 loops=1)
>    Recheck Cond: ((create_dt >= '2020-02-01 00:00:00-04'::timestamp with
> time zone) AND (create_dt < '2020-03-01 00:00:00-04'::timestamp with
> time zone))
>    Rows Removed by Index Recheck: 15936
>    Heap Blocks: lossy=15104
>    ->  Bitmap Index Scan on cd_single  (cost=0.00..27.78 rows=1267458
> width=0) (actual time=0.406..0.406 rows=151040 loops=1)
>          Index Cond: ((create_dt >= '2020-02-01 00:00:00-04'::timestamp
> with time zone) AND (create_dt < '2020-03-01 00:00:00-04'::timestamp
> with time zone))
>  Planning Time: 0.197 ms
>  Execution Time: 237.146 ms
> (8 rows)
> 
> 
> After delete, vacuum, and insert, BRIN multi is chosen over seq scan
> even though the correlation should be somewhat off (I didn't go further
> and try to find a case where seq scan is wrongly preferred):
> 
>                                                                        
>   QUERY PLAN
>
--------------------------------------------------------------------------------------------------------------------------------------------------------------
>  Bitmap Heap Scan on iot  (cost=428.53..247273.68 rows=1135074 width=57)
> (actual time=1.798..252.494 rows=1128038 loops=1)
>    Recheck Cond: ((create_dt >= '2020-02-01 00:00:00-04'::timestamp with
> time zone) AND (create_dt < '2020-03-01 00:00:00-04'::timestamp with
> time zone))
>    Rows Removed by Index Recheck: 140698
>    Heap Blocks: lossy=15104
>    ->  Bitmap Index Scan on cd_multi  (cost=0.00..144.76 rows=1598833
> width=0) (actual time=0.940..0.941 rows=151040 loops=1)
>          Index Cond: ((create_dt >= '2020-02-01 00:00:00-04'::timestamp
> with time zone) AND (create_dt < '2020-03-01 00:00:00-04'::timestamp
> with time zone))
>  Planning Time: 0.152 ms
>  Execution Time: 311.999 ms
> (8 rows)
> 
> 
> Add regular BRIN index, and it will get chosen, making the scan slower:
> 
>                                                                        
>   QUERY PLAN
>
--------------------------------------------------------------------------------------------------------------------------------------------------------------
>  Bitmap Heap Scan on iot  (cost=308.20..246966.38 rows=1118304 width=57)
> (actual time=5.685..1277.854 rows=1128038 loops=1)
>    Recheck Cond: ((create_dt >= '2020-02-01 00:00:00-04'::timestamp with
> time zone) AND (create_dt < '2020-03-01 00:00:00-04'::timestamp with
> time zone))
>    Rows Removed by Index Recheck: 7548826
>    Heap Blocks: lossy=103296
>    ->  Bitmap Index Scan on cd_single  (cost=0.00..28.62 rows=1551397
> width=0) (actual time=4.609..4.609 rows=1032960 loops=1)
>          Index Cond: ((create_dt >= '2020-02-01 00:00:00-04'::timestamp
> with time zone) AND (create_dt < '2020-03-01 00:00:00-04'::timestamp
> with time zone))
>  Planning Time: 0.211 ms
>  Execution Time: 1338.685 ms
> (8 rows)
> 
> Apply 0007 -- no difference:
> 
>                                                                        
>   QUERY PLAN
>
--------------------------------------------------------------------------------------------------------------------------------------------------------------
>  Bitmap Heap Scan on iot  (cost=308.20..246966.38 rows=1118304 width=57)
> (actual time=6.988..1358.113 rows=1128038 loops=1)
>    Recheck Cond: ((create_dt >= '2020-02-01 00:00:00-04'::timestamp with
> time zone) AND (create_dt < '2020-03-01 00:00:00-04'::timestamp with
> time zone))
>    Rows Removed by Index Recheck: 7548826
>    Heap Blocks: lossy=103296
>    ->  Bitmap Index Scan on cd_single  (cost=0.00..28.62 rows=1551397
> width=0) (actual time=5.528..5.528 rows=1032960 loops=1)
>          Index Cond: ((create_dt >= '2020-02-01 00:00:00-04'::timestamp
> with time zone) AND (create_dt < '2020-03-01 00:00:00-04'::timestamp
> with time zone))
>  Planning Time: 3.534 ms
>  Execution Time: 1418.194 ms
> (8 rows)
> 
> Apply 0008 -- now it chooses minmax-multi:
> 
>                                                                        
>   QUERY PLAN
>
--------------------------------------------------------------------------------------------------------------------------------------------------------------
>  Bitmap Heap Scan on iot  (cost=422.94..245412.66 rows=1118304 width=57)
> (actual time=2.750..259.850 rows=1128038 loops=1)
>    Recheck Cond: ((create_dt >= '2020-02-01 00:00:00-04'::timestamp with
> time zone) AND (create_dt < '2020-03-01 00:00:00-04'::timestamp with
> time zone))
>    Rows Removed by Index Recheck: 140698
>    Heap Blocks: lossy=15104
>    ->  Bitmap Index Scan on cd_multi  (cost=0.00..143.36 rows=1128092
> width=0) (actual time=1.609..1.609 rows=151040 loops=1)
>          Index Cond: ((create_dt >= '2020-02-01 00:00:00-04'::timestamp
> with time zone) AND (create_dt < '2020-03-01 00:00:00-04'::timestamp
> with time zone))
>  Planning Time: 1.421 ms
>  Execution Time: 319.891 ms
> (8 rows)
> 
> 
> So, 0007 doesn't make a difference in this case, but 0008 does.
> 

Interesting. Anyway, we picked the BRIN in both cases (over just using
seqscan), and it's unlikely to have two brin indexes with different
opclasses.

Barring objections, I'll get the 0001-0006 parts committed soon, and
then we can continue working on the costing changes for the next major
version.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 3/17/21 7:59 PM, John Naylor wrote:
> On Thu, Mar 11, 2021 at 12:26 PM Tomas Vondra
> <tomas.vondra@enterprisedb.com <mailto:tomas.vondra@enterprisedb.com>>
> wrote:
>>
>> Hi,
>>
>> Here is an updated version of the patch series.
>>
>> It fixes the assert failure (just remove the multiplication from it) and
>> adds a simple regression test that exercises this.
>>
>> Based on the discussion so far, I've decided to keep just the new
>> signature of the consistent function. That's a bit simpler than having
>> to support both 3 and 4 parameters, and it would not deal with the NULL
>> changes anyway (mostly harmless code duplication, but still). I've also
>> realized the API documentation in SGML needs updating.
>>
>> At this point, I think 0001-0006 parts are mostly committable.
> 
> I think so. I've marked it RFC for this six.
> 

I was just about to commit the 0001-0006 over the weekend. I went over
the patches to polish them, most of the changes were pretty simple:

- minor cleanup / rewording of comments

- resolving two remaining FIXMEs in the minmax-multi patch

- removing/rewording a bunch of XXX comments (most of them are about
possible future improvements)

- assigned proper OIDs and bumped catversion in patches touching the
catalogs

- bugfix: 0005 and 0006 were still adding fields into BrinOptions and
BrinDesc, but that was used before we got opclass parameters

- bugfix: two or three corrections in catalog contents

- doc fix: the brin.sgml bloom was referring to bit_bloom_ops, but
there's no such thing

- I have considered to get rid of 0004 (essentially merging it into 0002
and 0003 patches) but I decided not to do that, as it'd make the changes
in those two patches less obvious.



But then I remembered that long time ago there were suggestions to not
include the new opclasses directly, but through contrib. I'm not sure if
we want to do that, but I decided to give it a try - attached are the
polished patches 0001-0006, along with two patches that move the bloom
and minmax-multi opclasses to contrib.

In general, it seems much easier to define the opclasses in extension as
opposed to doing it directly in src/include/catalog. It however probably
needs a bit more work - in particular, the extensions currently create
just operator classes, not operator families. Not sure what consequences
could that have, though.

All the regression tests work fine, with the exception of minmax-multi
on a CIDR column. I don't know why, but the CREATE INDEX then fails like
this:

  ERROR:  missing operator 1(650,650) in opfamily 16463

650 is cidr, so this essentially says there's no (cidr<cidr) operator.
With the opclasses defined in .dat files this however worked, so I
suppose it's related to the missing operator families.

There's one remaining problem, though - the opclasses are using custom
data types for the range summaries. Essentially a varlena data type,
with internal structure. When defined directly in core, we knew the OID
of that type, which allowed us to store it into BrinOpcInfo. That's
mostly internal information, but pageinspect used it to print the
summary in brin_page_items (by just using the type out function).
Obviously, without the OID it prints just bytea blob, which is not very
useful :-(

I wonder if we could lookup the type, somehow. I mean, we know the name
of the types (brin_bloom_summary/brin_minmax_multi_summary), so perhaps
we could look it up in TYPENAMENSP? Not sure where to get the namespace,
though, and maybe there are (security) issues with this? Or maybe we
could pass the type OID to pageinspect directly (we're already passing
OID of the index, so we already have to trust the value).


regards


-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 3/22/21 6:27 AM, Tomas Vondra wrote:
> 
> ...
>
> All the regression tests work fine, with the exception of minmax-multi
> on a CIDR column. I don't know why, but the CREATE INDEX then fails like
> this:
> 
>   ERROR:  missing operator 1(650,650) in opfamily 16463
> 
> 650 is cidr, so this essentially says there's no (cidr<cidr) operator.
> With the opclasses defined in .dat files this however worked, so I
> suppose it's related to the missing operator families.
> 

Turns out this is likely a bug elsewhere. After a couple fixes to the
extension SQL script, the only real difference in catalog contents
(compared to e.g. the built-in BRIN minmax inet opclass) is that the
built-in one has opckeytype set to 869 (i.e. inet), while the one
created from extension has it set to 0.

The opclasses for inet (OID 869) look like this:

test=# select oid, opcname, opcfamily, opcintype, opckeytype from
pg_opclass where opcintype = 869 order by oid;
  oid  |        opcname        | opcfamily | opcintype | opckeytype
-------+-----------------------+-----------+-----------+------------
 10009 | cidr_ops              |      1974 |       869 |          0
 10010 | cidr_ops              |      1975 |       869 |          0
 10015 | inet_ops              |      1974 |       869 |          0
 10016 | inet_ops              |      1975 |       869 |          0
 10017 | inet_ops              |      3550 |       869 |          0
 10018 | inet_ops              |      3794 |       869 |          0
 10105 | inet_minmax_ops       |      4075 |       869 |        869
 10106 | inet_inclusion_ops    |      4102 |       869 |        869
 16451 | inet_bloom_ops        |     16450 |       869 |          0
 17398 | inet_minmax_multi_ops |     17397 |       869 |          0
(10 rows)

The only difference between the two minmax variants is the opckeytype,
and if I update it to 869 for inet_minmax_multi_ops, it starts working.
Likewise, if I set it to 0 for inet_minmax_ops, it breaks the same way.

Turns out, this is impossible to set from CREATE OPERATOR CLASS, because
opclasscmds.c does this:

    /* Just drop the spec if same as column datatype */
    if (storageoid == typeoid && false)
        storageoid = InvalidOid;

but index.c looks only at opclassTup->opckeytype. The built-in opclasses
don't have this issue, because they put the data into the catalog
directly, without going through this code.

I don't know what's the right fix, but it seems like this patch has
nothing to do with it. If we want to move the opclasses into an
extension, we can comment out that one (cidr/inet) case for now.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

I've pushed the first couple patches, reworking the BRIN interface and
the two existing opclasses. Attached are are the remaining bits,
implementing the two new opclasses.

0001 fixes the opckeytype I explained in the previous message. I'll
start a new thread, so that it does not get buried in this thread.

The 0002 and 0003 are the "main" patches, implementing all the stuff as
in-core opclasses. These patches were reviewed multiple times before, no
new changes.

0004 and 0005 are the patches moving the new opclasses to contrib. This
should also undo the catversion and OID generation changes from 0002 and
0003, but I'll take care of that if we actually decide contrib is the
right way. I kinda like it, if we can solve the two issues:

1) the opckeytype - I think this is a bug elsewhere, affecting any
opclass created by CREATE OPERATOR CLASS and not by directly injecting
the data into catalogs. I'll start a separate thread for that.

2) the pageinspect - Without knowing OID of the types used for summary,
brin_page_items can't print info about the bloom filter, minmax-multi
ranges, etc. Unfortunately, by moving the code to contrib we lose the
static OID assignment. I think there are three solutions to this:

a) Just use BYTEAOID, and accept that pageinspect prints just and
incomprehensible stream of characters

b) Use TypenameGetTypidExtended() to lookup the type by name. This is
what the code does now, but I'm afraid this might have security issues
due to search_path shenanigans. Base types can be created only by
superusers, but it's possible to create a composite type, and confuse
the lookup ...

c) Don't bother passing OID, and instead pass a pointer to the output
function directly. We'd need to extend BrinOpcInfo a bit, but that seems
like the simplest solution.

I think (b) is too dangerous/fragile, (a) is the simplest and (c) is a
bit too invasive/ugly I think.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2021-Mar-22, Tomas Vondra wrote:

> I don't know what's the right fix, but it seems like this patch has
> nothing to do with it. If we want to move the opclasses into an
> extension, we can comment out that one (cidr/inet) case for now.

I don't know what would be a good reason to define the opclasses in
separate contrib extensions.  I think it's going to be a nuisance to
users, so unless there is some strong argument for it, I'd suggest not
to do it.  I found it being discussed here:
https://www.postgresql.org/message-id/CA%2BTgmoajaQKBUx%3DvaTUFo6z80dsRzBw__Nu41Q4t06baZep3Ug%40mail.gmail.com
but there weren't any strong arguments put forward.

It seems a good experiment to have done it, though, since we now know
that there is a limitation in the existing SQL interface.  Maybe the fix
to that problem is to add a new clause to CREATE/ALTER OPERATOR CLASS to
let you define what goes into opckeytype.  However I don't think it's
this patch's responsibility to fix that problem.

-- 
Álvaro Herrera       Valdivia, Chile
"Hay que recordar que la existencia en el cosmos, y particularmente la
elaboración de civilizaciones dentro de él no son, por desgracia,
nada idílicas" (Ijon Tichy)

On 3/23/21 2:36 PM, Alvaro Herrera wrote:
> On 2021-Mar-22, Tomas Vondra wrote:
> 
>> I don't know what's the right fix, but it seems like this patch has
>> nothing to do with it. If we want to move the opclasses into an
>> extension, we can comment out that one (cidr/inet) case for now.
> 
> I don't know what would be a good reason to define the opclasses in
> separate contrib extensions.  I think it's going to be a nuisance to
> users, so unless there is some strong argument for it, I'd suggest not
> to do it.  I found it being discussed here:
> https://www.postgresql.org/message-id/CA%2BTgmoajaQKBUx%3DvaTUFo6z80dsRzBw__Nu41Q4t06baZep3Ug%40mail.gmail.com
> but there weren't any strong arguments put forward.
> 

OK, thanks for the opinion. Yeah, you're right there were no strong
opinions either way, and I see both pros/cons for the contrib option.
Defining the opclasses using SQL is way more convenient and less
error-prone than doing that directly in .dat files, for example. But
there are some limitations too, so not sure it's worth it.

> It seems a good experiment to have done it, though, since we now know
> that there is a limitation in the existing SQL interface.  Maybe the fix
> to that problem is to add a new clause to CREATE/ALTER OPERATOR CLASS to
> let you define what goes into opckeytype.  However I don't think it's
> this patch's responsibility to fix that problem.
> 

Yeah, it was a good experiment. I think we still need to figure out what
to do about the opckeytype - what needs to be fixed, exactly.

FWIW there's yet another difference between the current BRIN opclass
definition, compared to what CREATE OPERATOR CLASS would do. Or more
precisely, how we'd define opfamily for the cross-type cases (integer,
float and timestamp cases).

AFAICS we don't really need pg_amproc entries for the cross-type cases,
we just need the operators, so pg_amproc entries like

{ amprocfamily => 'brin/datetime_minmax_ops', amproclefttype =>
'timestamptz',
  amprocrighttype => 'timestamp', amprocnum => '1',
  amproc => 'brin_minmax_opcinfo' },

are unnecessary. The attached patch cleans that up, without breaking any
regression tests. Or is there a reason why we need those?

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2021-Mar-23, Tomas Vondra wrote:

> FWIW there's yet another difference between the current BRIN opclass
> definition, compared to what CREATE OPERATOR CLASS would do. Or more
> precisely, how we'd define opfamily for the cross-type cases (integer,
> float and timestamp cases).
> 
> AFAICS we don't really need pg_amproc entries for the cross-type cases,
> we just need the operators, so pg_amproc entries like
> 
> { amprocfamily => 'brin/datetime_minmax_ops', amproclefttype =>
> 'timestamptz',
>   amprocrighttype => 'timestamp', amprocnum => '1',
>   amproc => 'brin_minmax_opcinfo' },
> 
> are unnecessary. The attached patch cleans that up, without breaking any
> regression tests. Or is there a reason why we need those?

... ooh ...

When you say "just the operators" you mean the pg_amop entries, right?

I think I agree -- cross-type amproc entries are unlikely to have any
use.

-- 
Álvaro Herrera       Valdivia, Chile

On 3/23/21 7:28 PM, Alvaro Herrera wrote:
> On 2021-Mar-23, Tomas Vondra wrote:
> 
>> FWIW there's yet another difference between the current BRIN opclass
>> definition, compared to what CREATE OPERATOR CLASS would do. Or more
>> precisely, how we'd define opfamily for the cross-type cases (integer,
>> float and timestamp cases).
>>
>> AFAICS we don't really need pg_amproc entries for the cross-type cases,
>> we just need the operators, so pg_amproc entries like
>>
>> { amprocfamily => 'brin/datetime_minmax_ops', amproclefttype =>
>> 'timestamptz',
>>   amprocrighttype => 'timestamp', amprocnum => '1',
>>   amproc => 'brin_minmax_opcinfo' },
>>
>> are unnecessary. The attached patch cleans that up, without breaking any
>> regression tests. Or is there a reason why we need those?
> 
> ... ooh ...
> 
> When you say "just the operators" you mean the pg_amop entries, right?
> 
> I think I agree -- cross-type amproc entries are unlikely to have any
> use.
> 

I've pushed a cleanup of the unnecessary pg_amproc entries for the
built-in opclasses, and I have omitted them from the definition of the
new opclasses. Buildfarm seems happy, so far.

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Hi,

I've pushed both the bloom and minmax-multi indexes today.

Based on the feedback and limitations described before I decided to keep
them in core (i.e. not move them to contrib), but it was very useful
experiment as it uncovered a couple issues - both in existing code, and
in the definition of the new opclasses.

After further thought I've concluded that the decision to ditch the old
signature (in a1c649d889) was probably wrong, so I've added the support
back, per what Alexander originally proposed.

While doing so, I've noticed that the NULL handling may be a few bricks
shy, because with (oi_regular_nulls == false) it should probably keep
passing the NULL scan keys to the consistent function. I'll look into
that and get it fixed.


Thanks everyone who helped with those patches!

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2021-Mar-26, Tomas Vondra wrote:

> I've pushed both the bloom and minmax-multi indexes today.

Congratulations!  I think this reimplementation of the minmax opclass
infrastructure makes BRIN much more useful (read: actually usable).

-- 
Álvaro Herrera       Valdivia, Chile

On 3/26/21 2:08 PM, Tomas Vondra wrote:
> Hi,
> 
> I've pushed both the bloom and minmax-multi indexes today.
> 

Hmmm, I see a couple buildfarm animals are upset about the minmax-multi
patch. It does pass for me both on x86_64 and 32-bit ARM (rpi4), so I'm
unable to reproduce this. But most of the machines having issues seem to
be sparc variants, and snapper does this:


[New LWP 1471]
[Thread debugging using libthread_db enabled]
Using host libthread_db library "/lib/sparc-linux-gnu/libthread_db.so.1".
Core was generated by `postgres: parallel worker for PID 1350
                             '.
Program terminated with signal 10, Bus error.
#0  0x007167fc in int82gt (fcinfo=0xffcc66d0) at int8.c:399
399        int64        val1 = PG_GETARG_INT64(0);
#0  0x007167fc in int82gt (fcinfo=0xffcc66d0) at int8.c:399
#1  0x00887a94 in FunctionCall2Coll (flinfo=0xb81a2c, collation=0,
arg1=12242916, arg2=0) at fmgr.c:1163


I recall seeing "bus error" on sparc with other patches because of
alignment issues, so I wonder if this is what's happening here.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
> I recall seeing "bus error" on sparc with other patches because of
> alignment issues, so I wonder if this is what's happening here.

Try compiling with the address sanitizer enabled.  Per c.h,

 * Testing can be done with "-fsanitize=alignment -fsanitize-trap=alignment"
 * on clang, or "-fsanitize=alignment -fno-sanitize-recover=alignment" on gcc.

            regards, tom lane

On 3/26/21 2:55 PM, Tom Lane wrote:
> Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
>> I recall seeing "bus error" on sparc with other patches because of
>> alignment issues, so I wonder if this is what's happening here.
> 
> Try compiling with the address sanitizer enabled.  Per c.h,
> 
>  * Testing can be done with "-fsanitize=alignment -fsanitize-trap=alignment"
>  * on clang, or "-fsanitize=alignment -fno-sanitize-recover=alignment" on gcc.
> 

Bingo! I see the one failing x86_64 machine has -fsanitize=alignment.


thanks

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 3/26/21 3:04 PM, Tomas Vondra wrote:
> On 3/26/21 2:55 PM, Tom Lane wrote:
>> Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
>>> I recall seeing "bus error" on sparc with other patches because of
>>> alignment issues, so I wonder if this is what's happening here.
>>
>> Try compiling with the address sanitizer enabled.  Per c.h,
>>
>>  * Testing can be done with "-fsanitize=alignment -fsanitize-trap=alignment"
>>  * on clang, or "-fsanitize=alignment -fno-sanitize-recover=alignment" on gcc.
>>
> 
> Bingo! I see the one failing x86_64 machine has -fsanitize=alignment.
> 

Yeah, the deserialization is borked. It assumes it can just point into
the serialized representation of the summary, but that is "compacted" by
ignoring alignment. Hence the failures. For me it fails only for timetz
and interval types, but perhaps sparc is more sensitive, or maybe it's
about 32/64 bits too (the only backtrace I'm aware of is from snapper,
so assuming it's 32bits it'd make sense it fails on int8).

I have a feeling I made the same mistake in serialization of MCV stats
some time ago, shame on me. I'll get this fixed.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 3/26/21 3:45 PM, Tomas Vondra wrote:
> On 3/26/21 3:04 PM, Tomas Vondra wrote:
>> On 3/26/21 2:55 PM, Tom Lane wrote:
>>> Tomas Vondra <tomas.vondra@enterprisedb.com> writes:
>>>> I recall seeing "bus error" on sparc with other patches because of
>>>> alignment issues, so I wonder if this is what's happening here.
>>>
>>> Try compiling with the address sanitizer enabled.  Per c.h,
>>>
>>>  * Testing can be done with "-fsanitize=alignment -fsanitize-trap=alignment"
>>>  * on clang, or "-fsanitize=alignment -fno-sanitize-recover=alignment" on gcc.
>>>
>>
>> Bingo! I see the one failing x86_64 machine has -fsanitize=alignment.
>>
> 
> Yeah, the deserialization is borked. It assumes it can just point into
> the serialized representation of the summary, but that is "compacted" by
> ignoring alignment. Hence the failures. For me it fails only for timetz
> and interval types, but perhaps sparc is more sensitive, or maybe it's
> about 32/64 bits too (the only backtrace I'm aware of is from snapper,
> so assuming it's 32bits it'd make sense it fails on int8).
> 
> I have a feeling I made the same mistake in serialization of MCV stats
> some time ago, shame on me. I'll get this fixed.
> 

I've pushed a fix, ensuring proper alignment. There was a second issue
that'd affect big endian systems, so I fixed that too.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On 2021-Mar-26, Tomas Vondra wrote:

> Hi,
> 
> I've pushed both the bloom and minmax-multi indexes today.

One thing I've been wondering all along is how useful are these
BRIN-backed bloom indexes compared to contrib-supplied bloom indexes.
My guess is that the BRIN implementation has some advantage, or you
would not have worked so much on it.  But what is it?


Thanks

-- 
Álvaro Herrera       Valdivia, Chile

On 3/27/21 7:09 PM, Alvaro Herrera wrote:
> On 2021-Mar-26, Tomas Vondra wrote:
> 
>> Hi,
>>
>> I've pushed both the bloom and minmax-multi indexes today.
> 
> One thing I've been wondering all along is how useful are these
> BRIN-backed bloom indexes compared to contrib-supplied bloom indexes.
> My guess is that the BRIN implementation has some advantage, or you
> would not have worked so much on it.  But what is it?
> 

The contrib/bloom indexes are a completely different type of index. They
are not BRIN but a completely separate AM. The bloom filters are per-row
(so the index is larger than BRIN) and it's useful when you have table
with many attributes, and need to test various combinations of them.


create table t (a int, b int, c int);

insert into t select 10 * random(), 10 * random(), 10 * random()
  from generate_series(1,1000000) s(i);

analyze t;

create index bloom_idx on t using bloom (a,b,c)
  with (length=80, col1=4, col2=4, col3=4);

create index brin_bloom_idx on t using
  brin (a int4_bloom_ops, b int4_bloom_ops, c int4_bloom_ops);


test=# \di+
                          List of relations
 Schema |      Name      | Table | Access Method | Size  | Description
--------+----------------+-------+---------------+-------+-------------
 public | bloom_idx      | t     | bloom         | 15 MB |
 public | brin_bloom_idx | t     | brin          | 88 kB |
(2 rows)


So it's a completely different kind of animal, perhaps closer to btree
than to BRIN. I'm sure there are cases where contrib/bloom works better
than brin/bloom, but also the other way around.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company