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)