On 3/20/21 11:18 AM, Dilip Kumar wrote:
> On Sat, Mar 20, 2021 at 3:05 PM Tomas Vondra
> <tomas.vondra@enterprisedb.com> wrote:
>>
>> Hi,
>>
>> I think this bit in brin_tuple.c is wrong:
>>
>> ...
>> Form_pg_attribute att = TupleDescAttr(brdesc->bd_tupdesc,
>> keyno);
>> Datum cvalue = toast_compress_datum(value,
>> att->attcompression);
>>
>> The problem is that this is looking at the index descriptor (i.e. what
>> types are indexed) instead of the stored type. For BRIN those may be
>> only loosely related, which is why the code does this a couple lines above:
>>
>> /* We must look at the stored type, not at the index descriptor. */
>> TypeCacheEntry *atttype
>> = brdesc->bd_info[keyno]->oi_typcache[datumno];
>
> Ok, I was not aware of this.
>
Yeah, the BRIN internal structure is not obvious, and the fact that all
the built-in BRIN variants triggers the issue makes it harder to spot.
>> For the built-in BRIN opclasses this happens to work, because e.g.
>> minmax stores two values of the original type. But it may not work for
>> other out-of-core opclasses, and it certainly doesn't work for the new
>> BRIN opclasses (bloom and minmax-multi).
>
> Okay
>
>> Unfortunately, the only thing we have here is the type OID, so I guess
>> the only option is using GetDefaultToastCompression(). Perhaps we might
>> include that into BrinOpcInfo too, in the future.
>
> Right, I think for now we can use default compression for this case.
>
Good. I wonder if we might have "per type" preferred compression in the
future, which would address this. But for now just using the default
compression seems fine.
regards
--
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
