On Fri, Jul 26, 2024 at 9:50 AM Ajin Cherian <itsajin@gmail.com> wrote: >> > Please find v7 patch-set, the changes are: >
Thanks Ajin for working on this. Please find few comments:
1) parse_subscription_conflict_resolvers(): Here we loop in this function to find the given conflict type in the supported list and error out if conflict-type is not valid. Also we call validate_conflict_type_and_resolver() which again validates conflict-type. I would recommend to loop 'stmtresolvers' in parse function and then read each type and resolver and pass that to validate_conflict_type_and_resolver(). Avoid double validation.
I have modified this as per comment.
2) SetSubConflictResolver(): It works well, but it does not look apt that the 'resolvers' passed to this function by the caller is an array and this function knows the array range and traverse from CT_MIN to CT_MAX assuming this array maps directly to ConflictType. I think it would be better to have it passed as a list and then SetSubConflictResolver() traverse the list without knowing the range of it. Similar to what we do in alter-sub-flow in and around UpdateSubConflictResolvers().
I have kept the array as it requires that all conflict resolvers be set, if not provided by the user then default needs to be used. However, I have modified SetSubConflictResolver such that it takes in the size of the array and does not assume it.
3) When I execute 'alter subscription ..(detect_conflict=on)' for a subscription which *already* has detect_conflict as ON, it tries to reset resolvers to default and ends up in error. It should actually be no-op in this particular situation and should not reset resolvers to default.
postgres=# alter subscription sub1 set (detect_conflict=on); WARNING: Using default conflict resolvers ERROR: duplicate key value violates unique constraint "pg_subscription_conflict_sub_index"
fixed
4) Do we need SUBSCRIPTIONCONFLICTOID cache? We are not using it anywhere. Shall we remove this and the corresponding index?
We are using the index but not the cache, so removing the cache.
5) RemoveSubscriptionConflictBySubid(). --We can remove extra blank line before table_open. --We can get rid of curly braces around CatalogTupleDelete() as it is a single line in loop.
On Fri, Jul 26, 2024 at 9:50 AM Ajin Cherian <itsajin@gmail.com> wrote:
Comment in 0002,
1) I do not see any test case that set a proper conflict type and conflict resolver, all tests either give incorrect conflict type/conflict resolver or the conflict resolver is ignored
fixed.
I've also fixed a cfbot error due to patch 0001. Rebase of table resolver patch is still pending, will try and target that in the next patch-set.