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

On Tue, Jul 16, 2024 at 12:48 AM Nitin Motiani <nitinmotiani(at)google(dot)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.