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

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

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-04T07:22:51Z

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

Attachments

v12-0001-Distribute-invalidatons-if-change-in-catalog-tab.patch (application/octet-stream) patch v12-0001
v12-0002-Selective-Invalidation-of-Cache.patch (application/octet-stream) patch v12-0002

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.

Thanks and Regards,
Shlok Kyal