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

On Tue, Jul 16, 2024 at 9:29 AM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> 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()?
>

On my CentOS VM, the test file '100_bugs.pl' takes ~11s without a
patch and ~13.3s with a patch. So, 2 to 2.3s additional time for newly
added tests. It isn't worth adding this much extra time for one bug
fix. Can we combine table and schema tests into one single test and
avoid inheritance table tests as the code for those will mostly follow
the same path as a regular table?

--
With Regards,
Amit Kapila.