On Fri, Feb 5, 2021 at 6:45 PM Euler Taveira <eu...@eulerto.com> wrote: > > On Fri, Feb 5, 2021, at 4:01 AM, Amit Kapila wrote: > > I am not completely whether we should retire replorigin_drop or just > keep it for backward compatibility? What do you think? Anybody else > has any opinion? > > We could certainly keep some code for backward compatibility, however, we have > to consider if it is (a) an exposed API and/or (b) a critical path. We break > several extensions every release due to Postgres extensibility. For (a), it is > not an exposed function, I mean, we are not changing > `pg_replication_origin_drop`. Hence, there is no need to keep it. In (b), we > could risk slowing down some critical paths that we decide to keep the old > function and create a new one that contains additional features. It is not the > case for this function. It is rare that an extension does not have a few > #ifdef > if it supports multiple Postgres versions. IMO we should keep as little code > as > possible into the core in favor of maintainability. >
Yeah, that makes. I was a bit worried about pglogical but I think they can easily update it if required, so removed as per your suggestion. Petr, any opinion on this matter? I am planning to push this early next week (by Tuesday) unless you or someone else think it is not a good idea. > - replorigin_drop(roident, true); > + replorigin_drop_by_name(name, false /* missing_ok */ , true /* nowait */ ); > > A modern IDE would certainly show you the function definition that allows you > to check what each parameter value is without having to go back and forth. I > saw a few occurrences of this pattern in the source code and IMO it could be > used when it is not obvious what that value means. Booleans are easier to > figure out, however, sometimes integer and text are not. > Fair enough, removed in the attached patch. -- With Regards, Amit Kapila.
v4-0001-Make-pg_replication_origin_drop-safe-against-conc.patch
Description: Binary data
