On Fri, May 26, 2023 at 10:28 AM Peter Geoghegan <pg@bowt.ie> wrote:
> I've added several defensive assertions that make it hard to get the
> details wrong. These will catch the issue much earlier than the main
> "heapRel != NULL" assertion in _bt_allocbuf(). So, the rules are
> reasonably straightforward and enforceable.
Though it's not an issue new to 16, or a problem that anybody is
obligated to deal with on this thread, I wonder: Why is it okay that
we're using "rel" (the index rel) for our TestForOldSnapshot() calls
in nbtree, rather than using heapRel? Why wouldn't the rules be the
same as they are for the new code paths needed for logical decoding on
standbys? (Namely, the "use heapRel, not rel" rule.)
More concretely, I'm pretty sure that RelationIsUsedAsCatalogTable()
(which TestForOldSnapshot() relies on) gives an answer that would
change if we decided to pass heapRel to the main TestForOldSnapshot()
call within _bt_moveright(), instead of doing what we actually do,
which is to just pass it the index rel. I suppose that that
interaction might have been overlooked when bugfix commit bf9a60ee33
first added RelationIsUsedAsCatalogTable() -- since that happened a
couple of months after the initial "snapshot too old" commit went in,
a fix that happened under time pressure.
More generally, the high level rules/invariants that govern when
TestForOldSnapshot() should be called (and with what rel/snapshot)
feel less than worked out. I find it suspicious that there isn't any
attempt to relate TestForOldSnapshot() behaviors to the conceptually
similar PredicateLockPage() behavior. We don't need predicate locks on
internal pages, but TestForOldSnapshot() *does* get called for
internal pages. Many PredicateLockPage() calls happen very close to
TestForOldSnapshot() calls, each of which use the same snapshot -- not
addressing that seems like a glaring omission to me.
Basically it seems like there should be one standard set of rules for
all this stuff. Though it's not the fault of Bertrand or Andres, all
that we have now is two poorly documented sets of rules that partially
overlap. This has long bothered me.
--
Peter Geoghegan