Horton

Re: Inval reliability, especially for inplace updates

Noah Misch <noah@leadboat.com>

View thread
From: Noah Misch <noah@leadboat.com>
To: Nitin Motiani <nitinmotiani@google.com>, Andres Freund <andres@anarazel.de>, pgsql-hackers@postgresql.org
Date: 2024-10-24T02:53:57Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Update .abi-compliance-history for PrepareToInvalidateCacheTuple().

  2. Assert lack of hazardous buffer locks before possible catalog read.

  3. For inplace update, send nontransactional invalidations.

  4. Update .abi-compliance-history for CacheInvalidateHeapTupleInplace().

  5. Revisit cosmetics of "For inplace update, send nontransactional invalidations."

  6. Correct comments of "Fix data loss at inplace update after heap_update()".

  7. Move I/O before the index_update_stats() buffer lock region.

  8. Revert "For inplace update, send nontransactional invalidations."

  9. Revert "WAL-log inplace update before revealing it to other sessions."

  10. Fix inplace update buffer self-deadlock.

  11. Remove duplicate words in comments

  12. At end of recovery, reset all sinval-managed caches.

  13. Fix data loss at inplace update after heap_update().

  14. Remove comment about xl_heap_inplace "AT END OF STRUCT".

  15. Reduce memory consumption for pending invalidation messages.

  16. Add support for streaming to built-in logical replication.

  17. WAL Log invalidations at command end with wal_level=logical.

  18. Introduce logical decoding.

  19. Rename and document some invalidation routines to make it clearer that

Attachments

With the releases wrapping in 2.5 weeks, I'm ambivalent about pushing this
before the release or after.  Pushing before means fewer occurrences of
corruption, but pushing after gives more bake time to discover these changes
were defective.  It's hard to predict which helps users more, on a
risk-adjusted basis.  I'm leaning toward pushing this week.  Opinions?

On Sun, Oct 20, 2024 at 06:41:37PM +0530, Nitin Motiani wrote:
> I tested the patch locally and it works. And I have no other question
> regarding the structure. So this patch looks good to me to commit.

Thanks.  While resolving a back-branch merge conflict, I found
AtEOXact_Inval() and AtEOSubXact_Inval() were skipping inplaceInvalInfo tasks
if transInvalInfo==NULL.  If one PreInplace_Inval() failed, the session's next
inplace update would get an assertion failure.  Non-assert builds left
inplaceInvalInfo pointing to freed memory, but I didn't find a reachable
malfunction.  Separately, the xact.c comment edit wasn't reflecting that v4
brought back the transactional inval.  v6 fixes those.  Regarding the
back-branch alternatives to the WAL format change:

On Tue, Jun 18, 2024 at 08:23:49AM -0700, Noah Misch wrote:
> On Mon, Jun 17, 2024 at 06:57:30PM -0700, Andres Freund wrote:
> > On 2024-06-17 16:58:54 -0700, Noah Misch wrote:
> > > On Sat, Jun 15, 2024 at 03:37:18PM -0700, Noah Misch wrote:
> > > > - 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.
> > 
> > We already set that surprisingly frequently, as
> > a) The size of the sinval queue is small
> > b) If a backend is busy, it does not process catchup interrupts
> >    (i.e. executing queries, waiting for a lock prevents processing)
> > c) There's no deduplication of invals, we often end up sending the same inval
> >    over and over.
> > 
> > So I suspect this might not be too bad, compared to the current badness.
> 
> That is good.

I benchmarked that by hacking 027_stream_regress.pl to run "pgbench
--no-vacuum --client=4 -T 30 -b select-only" on the standby, concurrent with
the primary running the regression tests.  Standby clients acted on sinval
resetState ~16k times, and pgbench tps decreased 4.5%.  That doesn't
necessarily mean the real-life cost would be unacceptable, but it was enough
of a decrease that I switched to the next choice:

> We might be able to do the overflow signal once at end of
> recovery, like RelationCacheInitFileRemove() does for the init file.  That's
> mildly harder to reason about, but it would be cheaper.  Hmmm.

I'm attaching the branch-specific patches for that and for the main fix.
Other notes from back-patching:

- All branches change the ABI of PrepareToInvalidateCacheTuple(), a function
  catcache.c exports for the benefit of inval.c.  No PGXN extension calls
  that, and I can't think of a use case in extensions.

- Due to v15 commit 3aafc03, the patch for v14 differs to an unusual degree
  from the patch for v15+v16.  The difference is mostly mechanical, though.

Thanks,
nm