Dear Amit, Hou, Thanks for giving comments!
> For patch 0002, I think the implementation could be improved. The > current patch introduces a new function, RenamePublication, to replace the > existing generic approach in ExecRenameStmt->AlterObjectRename_internal. > However, this creates inconsistency because the original code uses > AccessExclusiveLock for publication renaming, making concurrent renaming > impossible. The current patch seems to overlook this aspect. Oh, I missed the point, thanks. > Additionally, introducing a new function results in code duplication, which > can > be avoided. After further consideration, handling the publication rename > separately seems unnecessary, given it requires only sending a few extra > invalidation messages. Therefore, I suggest reusing the generic handling and > simply extending AlterObjectRename_internal to include the additional > invalidation messages. > > I've attached a diff with the proposed changes for 0002. Hmm, possible. previously I introduced new function to preserve existing codes as much as possible. But this introduced some duplicy. Your approach which extends the common function looks nice, apart from two points; 1. Relsync cache invalidations should be done after the catalog update, but proposed one did before that. I preferred to add new if-statementfor post catalog-update. 2. Also, common check for the name conflict can be reused. Attached patch address comments by you and Amit [1]. What's new; * Skip adding new relsync messages when message for InvalidOid has already exist. Per comment from Amit [1]. * Update some code comments. Some of them are pointed out by [1]. * Stop using RenamePublication() and extend the common function. Best regards, Hayato Kuroda FUJITSU LIMITED
v10-0001-Introduce-a-new-invalidation-message-to-invalida.patch
Description: v10-0001-Introduce-a-new-invalidation-message-to-invalida.patch
v10-0002-Invalidate-Relcaches-while-ALTER-PUBLICATION-REN.patch
Description: v10-0002-Invalidate-Relcaches-while-ALTER-PUBLICATION-REN.patch
