Hi Kuroda-san, Thanks for reviewing the patch.
> > 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. > > On my environment, rel_sync_cache_publication_cb() was called first and > invalidate > all the entries, then rel_sync_cache_relation_cb() was called and the > specified > entry is invalidated - hence second is NO-OP. > You are correct. I made a silly mistake while writing the write-up. rel_sync_cache_publication_cb() is called first and invalidate all the entries, then rel_sync_cache_relation_cb() is called and the specified entry is invalidated > > This happens because of the following > > callback registered: > > CacheRegisterSyscacheCallback(PUBLICATIONRELMAP, > > rel_sync_cache_publication_cb, (Datum) 0); > > But even in this case, I could understand that you want to remove the > rel_sync_cache_publication_cb() callback. Yes, I think rel_sync_cache_publication_cb() callback can be removed, as it is invalidating all the other tables as well (which are not in this publication). > > 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. > > Even in this case, rel_sync_cache_publication_cb() was called first and then > rel_sync_cache_relation_cb(). > Yes, your observation is correct. rel_sync_cache_publication_cb() is called first and then rel_sync_cache_relation_cb(). > > > > 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. > > Hmm, I feel this fix is too much. Unlike ALTER PUBLICATION statements, I think > ALTER SCHEMA is rarely executed at the production stage. However, this > approach > requires adding a new cache callback system, which affects the entire postgres > system; this is not very beneficial compared to the outcome. It should be > discussed > on another thread to involve more people, and then we can add the improvement > after being accepted. > Yes, I also agree with you. I have removed the changes in the updated patch. > > 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. > > IIUC, the executed workload did not contain ALTER SCHEMA command, so > third improvement did not contribute this improvement. I have removed the changes corresponding to the third improvement. I have addressed the comment for 0002 patch and attached the patches. Also, I have moved the tests in the 0002 to 0001 patch. Thanks and Regards, Shlok Kyal
v10-0002-Selective-Invalidation-of-Cache.patch
Description: Binary data
v10-0001-Distribute-invalidatons-if-change-in-catalog-tab.patch
Description: Binary data
