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

> In the v7 patch, I am looping through the reorder buffer of the
> current committed transaction and storing all invalidation messages in
> a list. Then I am distributing those invalidations.
> But I found that for a transaction we already store all the
> invalidation messages (see [1]). So we don't need to loop through the
> reorder buffer and store the invalidations.
>
> I have modified the patch accordingly and attached the same.
>
> [1]: https://github.com/postgres/postgres/blob/7da1bdc2c2f17038f2ae1900be90a0d7b5e361e0/src/include/replication/reorderbuffer.h#L384

Hi,

I tried to add changes to selectively invalidate the cache to reduce
the performance degradation during the distribution of invalidations.

Here is the analysis for selective invalidation.
Observation:
Currently when there is a change in a publication, cache related to
all the tables is invalidated including the ones that are not part of
any publication and even tables of different publications. For
example, suppose pub1 includes tables t1 to t1000, while pub2 contains
just table t1001. If pub2 is altered, even though it only has t1001,
this change will also invalidate all the tables t1 through t1000 in
pub1.
Similarly for a namespace, whenever we alter a schema or we add/drop a
schema to the publication, cache related to all the tables is
invalidated including the ones that are on of different schema. For
example, suppose pub1 includes tables t1 to t1000 in schema sc1, while
pub2 contains just table t1001 in schema sc2. If schema ‘sc2’ is
changed or if it is dropped from publication ‘pub2’ even though it
only has t1001, this change will invalidate all the tables t1 through
t1000 in schema sc1.
‘rel_sync_cache_publication_cb’ function is called during the
execution of invalidation in both above cases. And
‘rel_sync_cache_publication_cb’ invalidates all the tables in the
cache.

Solution:
1. When we alter a publication using commands like ‘ALTER PUBLICATION
pub_name DROP TABLE table_name’, first all tables in the publications
are invalidated using the function ‘rel_sync_cache_relation_cb’. Then
again ‘rel_sync_cache_publication_cb’ function is called which
invalidates all the tables. This happens because of the following
callback registered:
CacheRegisterSyscacheCallback(PUBLICATIONRELMAP,
rel_sync_cache_publication_cb, (Datum) 0);

So, I feel this second function call can be avoided. And I have
included changes for the same in the patch. Now the behavior will be
as:
suppose pub1 includes tables t1 to t1000, while pub2 contains just
table t1001. If pub2 is altered, it will only invalidate t1001.

2. When we add/drop a schema to/from a publication using command like
‘ALTER PUBLICATION pub_name ADD TABLES in SCHEMA schema_name’, first
all tables in that schema are invalidated using
‘rel_sync_cache_relation_cb’ and then again
‘rel_sync_cache_publication_cb’ function is called which invalidates
all the tables. This happens because of the following callback
registered:
CacheRegisterSyscacheCallback(PUBLICATIONNAMESPACEMAP,
rel_sync_cache_publication_cb, (Datum) 0);

So, I feel this second function call can be avoided. And I have
included changes for the same in the patch. Now the behavior will be
as:
suppose pub1 includes tables t1 to t1000 in schema sc1, while pub2
contains just table t1001 in schema sc2. If schema ‘sc2’ dropped from
publication ‘pub2’, it will only invalidate table t1001.

3. When we alter a namespace using command like ‘ALTER SCHEMA
schema_name RENAME to new_schema_name’ all the table in cache are
invalidated as ‘rel_sync_cache_publication_cb’ is called due to the
following registered callback:
CacheRegisterSyscacheCallback(NAMESPACEOID,
rel_sync_cache_publication_cb, (Datum) 0);

So, we added a new callback function ‘rel_sync_cache_namespacerel_cb’
will be called instead of function ‘rel_sync_cache_publication_cb’ ,
which invalidates only the cache of the tables which are part of that
particular namespace. For the new function the ‘namespace id’ is added
in the Invalidation message.

For example, if namespace ‘sc1’ has table t1 and t2 and a namespace
‘sc2’ has table t3. Then if we rename namespace ‘sc1’ to ‘sc_new’.
Only tables in sc1 i.e. tables t1 and table t2 are invalidated.


Performance Comparison:
I have run the same tests as shared in [1] and observed a significant
decrease in the degradation with the new changes.  With selective
invalidation degradation is around ~5%. This results are an average of
3 runs.

count     |      Head (sec)      |        Fix (sec)      |     Degradation (%)
-----------------------------------------------------------------------------------------
10000    |    0.38842567       |    0.405057        |     4.281727827
50000    |    7.22018834       |   7.605011334    |     5.329819333
75000    |    15.627181         |   16.38659034    |     4.859541462
100000  |     27.37910867    |    28.8636873     |     5.422304458

I have attached the patch for the same
v9-0001 : distribute invalidation to inprogress transaction
v9-0002: Selective invalidation

[1]:https://www.postgresql.org/message-id/CANhcyEW4pq6%2BPO_eFn2q%3D23sgV1budN3y4SxpYBaKMJNADSDuA%40mail.gmail.com


Thanks and Regards,
Shlok Kyal