Re: use indnkeyatts not indnatts in loops that read rd_indcollation

On Fri, 9 Oct 2026 at 10:41, jian he <jian(dot)universality(at)gmail(dot)com> wrote:
>
> Hi
>
> While reivew https://www.postgresql.org/message-id/CAGRkXqR7VXacmRP41UBaM5u4aseeCH5ECVmQgn_YS%3DL7%3DxdhTw%40mail.gmail.com
> I accidentally made Claude discover this issue. (At that time, I was
> asking Claude about a collation-related issue.)
>
> In RelationInitIndexAccessInfo, we have
> ```
> relation->rd_indcollation = (Oid *)
> MemoryContextAllocZero(indexcxt, indnkeyatts * sizeof(Oid));
> ```
> rd_indcollation is corresponds to indnkeyatts, *not* indnatts.
> Therefore any rd_indcollation related places loop using indnatts if
> is there is wrong.
> For example, below infer_collation_opclass_match is wrong.
> ```
> for (natt = 1; natt <= idxRel->rd_att->natts; natt++)
> ```
>
> Some index type does not support INCLUDE index, but to be
> future-proof, if somewhere rd_indcollation related, loop should using
> indnkeyatts.

I think the changes to BRIN are meaningless churn; it can never
support INCLUDE columns.

GIN could eventually support amgettuple again (which was removed way
back when the pending list was introduced, with ff301d6e690b), and
with even more effort and community goodwill might eventually get
INCLUDE support, so changing its code is not the worst choice, but
feels a lot like it's just churn to get it in line with other AMs.

Apart from that commentary, LGTM.

Kind regards,

Matthias van de Meent
Databricks (https://www.databricks.com)

PS. Note that relcache.c's write_relcache_init_file() *also* uses
natts, not nkeyatts, for the length of these nkeyatts-sized arrays.
My patch at [0] includes a fix for that.
[0] https://commitfest.postgresql.org/patch/7237/