Re: pg_replication_origin_drop API potential race condition

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.

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

--
Euler Taveira
EDB https://www.enterprisedb.com/