Re: pg_replication_origin_drop API potential race condition

On 06/02/2021 07:29, Amit Kapila wrote:
> 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.

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