On Fri, Jul 26, 2024 at 9:50 AM Ajin Cherian <itsa...@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. 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(). 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" 4) Do we need SUBSCRIPTIONCONFLICTOID cache? We are not using it anywhere. Shall we remove this and the corresponding index? 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. thanks Shveta
