Re: long-standing data loss bug in initial sync of logical replication

Amit Kapila <amit.kapila16@gmail.com>

From: Amit Kapila <amit.kapila16@gmail.com>

To: Shlok Kyal <shlok.kyal.oss@gmail.com>

Cc: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Masahiko Sawada <sawada.mshk@gmail.com>, vignesh C <vignesh21@gmail.com>, Nitin Motiani <nitinmotiani@google.com>, Andres Freund <andres@anarazel.de>, PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>

Date: 2025-02-28T11:17:50Z

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 →

Fix typo in test file name added in commit 4909b38af0.
- 50b8ad30f754 18.0 landed
- d96206f259d6 17.5 landed
- 9987c94662c2 16.9 landed
- 90bc4523fd47 15.13 landed
- bb1bc9fa962e 14.18 landed
- 4164d6976316 13.21 landed
Fix data loss in logical replication.
- 247ee94150b6 13.21 landed
- 4909b38af034 18.0 landed
- cadaf0ac4637 17.5 landed
- 9a2f8b4f01d5 16.9 landed
- 9f21be08e884 15.13 landed
- 0434033e8bb5 14.18 landed
Avoid invalidating all RelationSyncCache entries on publication rename.
- 3abe9dc18892 18.0 cited
Remove obsolete RECHECK keyword completely
- 7da1bdc2c2f1 18.0 cited
Backport BackgroundPsql perl test module
- 187b8991f70f 16.4 cited

On Mon, Feb 24, 2025 at 4:49 PM Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> Patches need a rebase. Attached the rebased patch.
>

I would like to discuss 0002 patch:
 publication_invalidation_cb(Datum arg, int cacheid, uint32 hashvalue)
 {
  publications_valid = false;
-
- /*
- * Also invalidate per-relation cache so that next time the filtering info
- * is checked it will be updated with the new publication settings.
- */
- rel_sync_cache_publication_cb(arg, cacheid, hashvalue);
 }

 /*
@@ -1970,18 +1964,6 @@ init_rel_sync_cache(MemoryContext cachectx)
    rel_sync_cache_publication_cb,
    (Datum) 0);

- /*
- * Flush all cache entries after any publication changes.  (We need no
- * callback entry for pg_publication, because publication_invalidation_cb
- * will take care of it.)
- */
- CacheRegisterSyscacheCallback(PUBLICATIONRELMAP,
-   rel_sync_cache_publication_cb,
-   (Datum) 0);
- CacheRegisterSyscacheCallback(PUBLICATIONNAMESPACEMAP,
-   rel_sync_cache_publication_cb,
-   (Datum) 0);

In 0002 patch, we are improving the performance by avoiding
invalidation processing in a number of cases. Basically, the claim is
that we are unnecessarily invalidating all the RelSyncCache entries
when a particular relation's entry could be invalidated. I have not
verified it, but IIUC, this should be an independent improvement atop
HEAD; if so, then we should start a separate thread to discuss it.

Thoughts?

-- 
With Regards,
Amit Kapila.