On Thu, Feb 17, 2022, at 3:36 AM, Amit Kapila wrote: > As there is a new version, I would like to wait for a few more days > before committing. I am planning to commit this early next week (by > Tuesday) unless others or I see any more things that can be improved. Amit, I don't have additional comments or suggestions. Let's move on. Next topic. :-)
> I would once like to mention the replica identity handling of the > patch. Right now, (on HEAD) we are not checking the replica identity > combination at DDL time, they are checked at execution time in > CheckCmdReplicaIdentity(). This patch follows the same scheme and > gives an error at the time of update/delete if the table publishes > update/delete and the publication(s) has a row filter that contains > non-replica-identity columns. We had earlier thought of handling it at > DDL time but that won't follow the existing scheme and has a lot of > complications as explained in emails [1][2]. Do let me know if you see > any problem here? IMO it is not an issue that this patch needs to solve. The conclusion of checking the RI at the DDL time vs execution time is that: * the current patch just follows the same pattern used in the current logical replication implementation; * it is easier to check during execution time (a central point) versus a lot of combinations for DDL commands; * the check during DDL time might eventually break if new subcommands are added; * the execution time does not have the maintenance burden imposed by new DDL subcommands; * we might change the RI check to execute at DDL time if the current implementation imposes a significant penalty in certain workloads. Again, it is material for another patch. Thanks for taking care of a feature that has been discussed for 4 years [1]. [1] https://www.postgresql.org/message-id/CAHE3wggb715X%2BmK_DitLXF25B%3DjE6xyNCH4YOwM860JR7HarGQ%40mail.gmail.com -- Euler Taveira EDB https://www.enterprisedb.com/
