Re: Cache invalidation bug in RelationGetIndexAttrBitmap()

On 2014-05-14 13:32:43 -0400, Tom Lane wrote:
> Andres Freund <andres(at)2ndquadrant(dot)com> writes:
> > On 2014-05-14 12:15:27 -0400, Tom Lane wrote:
> >> And why does the header
> >> comment for RelationGetIndexList make no mention of this new side-effect?
> >> Somebody did a seriously poor job of adding this functionality to
> >> relcache.
>
> > It's not like it's not documented: There's a comment about it in struct
> > RelationData. I must have missed that rd_oidindex has a comment abou
> > it's lifetime over RelationGetIndexList().
>
> If rd_replidindex is being managed like rd_oidindex, then it should be
> managed just like rd_oidindex, including getting reset in all the places
> rd_oidindex is. This might be just a matter of cleanliness but I think
> it's important for readability and debuggability.

Agreed. I am not against resetting it. I think I might not have been
aware of rd_oidindex when writing that code...

> I notice also that rd_keyattr and rd_idattr have been implemented with
> bad copies of the logic for rd_indexattr. This is at least leading
> to a permanent memory leak in CacheMemoryContext during every relcache
> flush, and maybe worse things. The bugs for rd_keyattr appear to predate
> your patch though.

Hm. Yes, the bitmapsets should be freed. I guess I copied the logic for
keyattr and didn't find any relevant places that touch it. rd_keyattr
should go back to 9.3.

> Working on a patch for this now. One thing I'm wondering about is
> RelationSetIndexList. It's probably okay for it not to touch rd_keyattr
> and rd_idattr, but I'm not too clear on what the use cases for those
> attnum sets are.

rd_keyattr is used to determine whether a heap_update() changed any keys
that could be referenced by a foreign key. That's then used to determine
which locklevel an update requires.
rd_idattr does something similar. It decides whether the configured
REPLICA IDENTITY key has changed so whether to log the old primary key
for logical decoding or not.

I can't see why either would need to care about forced index lists right
now, but will do a scan of the sources to see if I am wrong.

Greetings,

Andres Freund

--
Andres Freund http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services