Re: pg_replication_origin_drop API potential race condition

On Fri, Feb 5, 2021 at 6:45 PM Euler Taveira <euler(at)eulerto(dot)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.