On 06/02/2021 07:29, Amit Kapila wrote:
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.
To be fair the logical replication framework is full of these comments
so it's pretty natural to add them to new code as well, but I agree with
Euler that it's unnecessary with any reasonable development tooling.
The patch as posted looks good to me, as an extension author I normally
have origin cached by id, so the api change means I have to do name
lookup now, but given this is just for drop, it does not really matter.
--
Petr
