Horton

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

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

View thread
From: Shlok Kyal <shlok.kyal.oss@gmail.com>
To: "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>
Cc: 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>, Amit Kapila <amit.kapila16@gmail.com>
Date: 2024-10-03T05:42:47Z
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. Fix typo in test file name added in commit 4909b38af0.

  2. Fix data loss in logical replication.

  3. Avoid invalidating all RelationSyncCache entries on publication rename.

  4. Remove obsolete RECHECK keyword completely

  5. Backport BackgroundPsql perl test module

Attachments

> > I have addressed the comment for 0002 patch and attached the patches.
> > Also, I have moved the tests in the 0002 to 0001 patch.
>
> Thanks for updating the patch. 0002 patch seems to remove cache invalidations
> from publication_invalidation_cb(). Related with it, I found an issue and had a concern.
>
> 1.
> The replication continues even after ALTER PUBLICATION RENAME is executed.
> For example - assuming that a subscriber subscribes only "pub":
>
> ```
> pub=# INSERT INTO tab values (1);
> INSERT 0 1
> pub=# ALTER PUBLICATION pub RENAME TO pub1;
> ALTER PUBLICATION
> pub=# INSERT INTO tab values (2);
> INSERT 0 1
>
> sub=# SELECT * FROM tab ; -- (2) should not be replicated however...
>  a
> ---
>  1
>  2
> (2 rows)
> ```
>
> This happens because 1) ALTER PUBLICATION RENAME statement won't be invalidate the
> relation cache, and 2) publications are reloaded only when invalid RelationSyncEntry
> is found. In given example, the first INSERT creates the valid cache and second
> INSERT reuses it. Therefore, the pubname-check is skipped.
>
> For now, the actual renaming is done at AlterObjectRename_internal(), a generic
> function. I think we must implement a dedecated function to publication and must
> invalidate relcaches there.
>
> 2.
> Similarly with above, the relcache won't be invalidated when ALTER PUBLICATION
> OWNER TO is executed. This means that privilage checks may be ignored if the entry
> is still valid. Not sure, but is there a possibility this causes an inconsistency?
>

Hi Kuroda-san,

Thanks for testing the patch. I have fixed the comments and attached
the updated patch.
I have added a callback function rel_sync_cache_publicationrel_cb().
This callback invalidates the cache of tables in a particular
publication.
This callback is called when there is some modification in
pg_publication catalog.

I have tested the two cases  'ALTER PUBLICATION ... RENAME TO ...' and
 'ALTER PUBLICATION ... OWNER TO ...'  and debugged it. The newly
added callback is called and it invalidates the cache of tables
present in that particular publication.
I have also added a test related to 'ALTER PUBLICATION ... RENAME TO
...' to 0001 patch.

Thanks and Regards,
Shlok Kyal