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