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

On Tue, Jul 16, 2024 at 12:48 AM Nitin Motiani <nitinmotiani@google.com> wrote:
>
> A couple of questions on the latest patch :
>
> 1. I see there is this logic in PublicationDropSchemas to first check
> if there is a valid entry for the schema in pg_publication_namespace
>
>                 psid = GetSysCacheOid2(PUBLICATIONNAMESPACEMAP,
>
> Anum_pg_publication_namespace_oid,
>
> ObjectIdGetDatum(schemaid),
>
> ObjectIdGetDatum(pubid));
>                 if (!OidIsValid(psid))
>                 {
>                         if (missing_ok)
>                                 continue;
>
>                         ereport(ERROR,
>                                         (errcode(ERRCODE_UNDEFINED_OBJECT),
>                                          errmsg("tables from schema
> \"%s\" are not part of the publication",
>
> get_namespace_name(schemaid))));
>                 }
>
> Your proposed change locks the schemaRels before this code block.
> Would it be better to lock the schemaRels after the error check? So
> that just in case, the publication on the schema is not valid anymore,
> the lock is not held unnecessarily on all its tables.
>

Good point. It is better to lock the relations in
RemovePublicationSchemaById() where we are invalidating relcache as
well. See the response to your next point as well.

> 2. The function publication_add_schema explicitly invalidates cache by
> calling InvalidatePublicationRels(schemaRels). That is not present in
> the current PublicationDropSchemas code. Is that something which
> should be added in the drop scenario also? Please let me know if there
> is some context that I'm missing regarding why this was not added
> originally for the drop scenario.
>

The required invalidation happens in the function
RemovePublicationSchemaById(). So, we should lock in
RemovePublicationSchemaById() as that would avoid calling
GetSchemaPublicationRelations() multiple times.

One related comment:
@@ -1219,8 +1219,14 @@ AlterPublicationTables(AlterPublicationStmt
*stmt, HeapTuple tup,
  oldrel = palloc(sizeof(PublicationRelInfo));
  oldrel->whereClause = NULL;
  oldrel->columns = NIL;
+
+ /*
+ * Data loss due to concurrency issues are avoided by locking
+ * the relation in ShareRowExclusiveLock as described atop
+ * OpenTableList.
+ */
  oldrel->relation = table_open(oldrelid,
-   ShareUpdateExclusiveLock);
+   ShareRowExclusiveLock);

Isn't it better to lock the required relations in RemovePublicationRelById()?

-- 
With Regards,
Amit Kapila.