Re: Inval reliability, especially for inplace updates

On Sat, Jun 15, 2024 at 03:37:18PM -0700, Noah Misch wrote:
> On Wed, May 22, 2024 at 05:05:48PM -0700, Noah Misch wrote:
> > https://postgr.es/m/20240512232923.aa.nmisch@google.com wrote:
> > > Separable, nontrivial things not fixed in the attached patch stack:
> > >
> > > - Inplace update uses transactional CacheInvalidateHeapTuple(). ROLLBACK of
> > > CREATE INDEX wrongly discards the inval, leading to the relhasindex=t loss
> > > still seen in inplace-inval.spec. CacheInvalidateRelmap() does this right.
> >
> > I plan to fix that like CacheInvalidateRelmap(): send the inval immediately,
> > inside the critical section. Send it in heap_xlog_inplace(), too.
>
> > a. Within logical decoding, cease processing invalidations for inplace
>
> I'm attaching the implementation. This applies atop the v3 patch stack from
> https://postgr.es/m/20240614003549.c2.nmisch@google.com, but the threads are
> mostly orthogonal and intended for independent review. Translating a tuple
> into inval messages uses more infrastructure than relmapper, which needs just
> a database ID. Hence, this ended up more like a miniature of inval.c's
> participation in the transaction commit sequence.
>
> I waffled on whether to back-patch inplace150-inval-durability-atcommit

That inplace150 patch turned out to be unnecessary. Contrary to the
"noncritical resource releasing" comment some lines above
AtEOXact_Inval(true), the actual behavior is already to promote ERROR to
PANIC. An ERROR just before or after sending invals becomes PANIC, "cannot
abort transaction %u, it was already committed". Since
inplace130-AtEOXact_RelationCache-comments existed to clear the way for
inplace150, inplace130 also becomes unnecessary. I've removed both from the
attached v2 patch stack.

> The patch changes xl_heap_inplace of XLOG_HEAP_INPLACE. For back branches, we
> could choose between:
>
> - Same change, no WAL version bump. Standby must update before primary. This
> is best long-term, but the transition is more disruptive. I'm leaning
> toward this one, but the second option isn't bad:
>
> - heap_xlog_inplace() could set the shared-inval-queue overflow signal on
> every backend. This is more wasteful, but inplace updates might be rare
> enough (~once per VACUUM) to make it tolerable.
>
> - Use LogStandbyInvalidations() just after XLOG_HEAP_INPLACE. This isn't
> correct if one ends recovery between the two records, but you'd need to be
> unlucky to notice. Noticing would need a procedure like the following. A
> hot standby backend populates a relcache entry, then does DDL on the rel
> after recovery ends.

That still holds.