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-07T11:15:03Z
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

On Fri, 4 Oct 2024 at 12:52, Shlok Kyal <shlok.kyal.oss@gmail.com> wrote:
>
> Hi Kuroda-san,
>
> Thanks for reviewing the patch.
> >
> > 1.
> > I feel the name of SnapBuildDistributeNewCatalogSnapshot() should be updated because it
> > distributes two objects: catalog snapshot and invalidation messages. Do you have good one
> > in your mind? I considered "SnapBuildDistributeNewCatalogSnapshotAndInValidations" or
> > "SnapBuildDistributeItems" but seems not good :-(.
>
> I have renamed the function to 'SnapBuildDistributeSnapshotAndInval'. Thoughts?
>
> > 2.
> > Hmm, still, it is overengineering for me to add a new type of invalidation message
> > only for the publication. According to the ExecRenameStmt() we can implement an
> > arbitrary rename function like RenameConstraint() and RenameDatabase().
> > Regaring the ALTER PUBLICATION OWNER TO, I feel adding CacheInvalidateRelcacheAll()
> > and InvalidatePublicationRels() is enough.
>
> I agree with you.
>
> >
> > I attached a PoC which implements above. It could pass tests on my env. Could you
> > please see it tell me how you think?
>
> I have tested the POC and it is working as expected. The changes look
> fine to me. I have created a patch for the same.
> Currently, we are passing 'PUBLICATION_PART_ALL' as an argument to
> function 'GetPublicationRelations' and
> 'GetAllSchemaPublicationRelations'. Need to check if we can use
> 'PUBLICATION_PART_ROOT' or 'PUBLICATION_PART_LEAF' depending on the
> 'publish_via_partition_root' option. Will test and address this in the
> next version of the patch. For now, I have added a TODO.

I have tested this part. I observed that ,whenever we insert data in a
partition table, the function 'get_rel_sync_entry' is called and a
hash entry is created for the corresponding leaf node relid. So I feel
while invalidating here we can specify 'PUBLICATION_PART_LEAF' . I
have made the corresponding changes 0002 patch.

I have also modified the tests in 0001 patch. These changes are only
related to syntax of writing tests.

Thanks and Regards,
Shlok Kyal