Relcache refactoring

While looking at the recent bug report from Alexander Lakhin [1], I got 
annoyed by the relcache code, and RelationClearRelation in particular. I 
propose to refactor it for clarity.

[1] 
https://www.postgresql.org/message-id/e56be7d9-14b1-664d-0bfc-00ce9772721c%40gmail.com

## Patch 1

This is just a narrow fix for the reported bug [1], same as I posted on 
that thread. Included here because I wrote the refactorings on top of 
this patch and didn't commit it yet.


## Patch 2: Simplify call to rebuild relcache entry for indexes

To rebuild a relcache entry that's been marked as invalid, 
RelationIdGetRelation() calls RelationReloadIndexInfo() for indexes and 
RelationClearRelation(rebuild == true) for other relations. However, 
RelationClearRelation calls RelationReloadIndexInfo() for indexes 
anyway, so RelationIdGetRelation() can just always call 
RelationClearRelation() and let RelationClearRelation() do the right 
thing to rebuild the relation, whether it's an index or something else. 
That seems more straightforward.

Also add comments explaining how the rebuild works at index creation. 
It's a bit special, see the comments.


## Patch 3: Split RelationClearRelation into three different functions

RelationClearRelation() is complicated. Depending on the 'rebuild' 
argument and the circumstances, like if it's called in a transaction and 
whether the relation is an index, a nailed relation, a regular table, or 
a relation dropped in the same xact, it does different things:

- Remove the relation completely from the cache (rebuild == false),
- Mark the entry as invalid (rebuild == true, but not in xact), or
- Rebuild the entry (rebuild == true).

The callers have expectations on what they want it to do. Mostly the 
callers with 'rebuild == false' expect the entry to be removed, and 
callers with 'rebuild == true' expect it to be rebuilt or invalidated, 
but there are exceptions. RelationForgetRelation() for example sets 
rd_droppedSubid and expects RelationClearRelation() to then merely 
invalidate it, and the call from RelationIdGetRelation() expects it to 
rebuild, not merely invalidate it.

I propose to split RelationClearRelation() into three functions:

RelationInvalidateRelation: mark the relcache entry as invalid, so that 
it it is rebuilt on next access.
RelationRebuildRelation: rebuild the relcache entry in-place.
RelationClearRelation: Remove the entry from the relcache.

This moves the responsibility of deciding the right action to the 
callers. Which they were mostly already doing. Each of those actions 
have different preconditions, e.g. RelationRebuildRelation() can only be 
called in a valid transaction, and RelationClearRelation() can only be 
called if the reference count is zero. Splitting them makes those 
preconditions more clear, we can have assertions to document them in each.


## RelationInitPhysicalAddr() call in RelationReloadNailed()

One question or little doubt I have: Before these patches, 
RelationReloadNailed() calls RelationInitPhysicalAddr() even when it 
leaves the relation as invalidated because we're not in a transaction or 
if the relation isn't currently in use. That's a bit surprising, I'd 
expect it to be done when the entry is reloaded, not when it's 
invalidated. That's how it's done for non-nailed relations. And in fact, 
for a nailed relation, RelationInitPhysicalAddr() is called *again* when 
it's reloaded.

Is it important? Before commit a54e1f1587, nailed non-index relations 
were not reloaded at all, except for the call to 
RelationInitPhysicalAddr(), which seemed consistent. I think this was 
unintentional in commit a54e1f1587, or perhaps just overly defensive, as 
there's no harm in some extra RelationInitPhysicalAddr() calls.

This patch removes that extra call from the invalidation path, but if it 
turns out to be wrong, we can easily add it to RelationInvalidateRelation.

-- 
Heikki Linnakangas
Neon (https://neon.tech)