Re: SQL:2011 application time

On 22.11.24 01:30, Paul Jungwirth wrote:
>> -       return get_equal_strategy_number_for_am(am);
>> +       /* For GiST indexes we need to ask the opclass what strategy
>> number to use. */
>> +       if (am == GIST_AM_OID)
>> +               return GistTranslateStratnum(opclass,
>> RTEqualStrategyNumber);
>> +       else
>> +               return get_equal_strategy_number_for_am(am);
>>
>> This code should probably be pushed into
>> get_equal_strategy_number_for_am().  That function already
>> has a switch based on index AM OIDs.  Also, there are other callers of
>> get_equal_strategy_number_for_am(), which also might want to become
>> aware of GiST support.
>
> Done. The reason I didn't do this before is because we need the opclass,
> not just the am. I put an
> explanation about that into the function comment. If that's a problem I
> can undo the change.

I think this is the right idea, but after digging around a bit more, I
think more could/should be done.

After these changes, the difference between
get_equal_strategy_number_for_am() and get_equal_strategy_number() is
kind of pointless. We should really just use
get_equal_strategy_number() for all purposes.

But then you have the problem that IsIndexUsableForReplicaIdentityFull()
doesn't have the opclass IDs available in the IndexInfo structure. You
appear to have worked around that by writing

+ if (get_equal_strategy_number_for_am(indexInfo->ii_Am, InvalidOid)
== InvalidStrategy)

which I suppose will have the same ultimate result as before that patch,
but it seems kind of incomplete.

I figure this could all be simpler if
IsIndexUsableForReplicaIdentityFull() used the index relcache entry
directly instead of going the detour through IndexInfo. Then we have
all the information available, and this should ultimately all work
properly for suitable GiST indexes as well.

I have attached three patches that show how that could be done. (This
would work in conjunction with your new tests. (Although now we could
also test GiST with replica identity full?))

The comment block for IsIndexUsableForReplicaIdentityFull() makes a
bunch of claims that are not all explicitly supported by the code. The
code doesn't actually check the AM, this is all only done indirectly via
other checks. The second point (about tuples_equal()) appears to be
slightly wrong, because while you need an equals operator from the type
cache, that shouldn't prevent you from also using a different index AM
than btree or hash for the replica identity index. And the stuff about
amgettuple, if that is important, why is it only checked for assert builds?