Re: [HACKERS] Custom compression methods

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