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].