On Sat, Feb 6, 2021 at 3:26 PM Petr Jelinek <pjmodos@pjmodos.net> wrote:
>
> On 06/02/2021 07:29, Amit Kapila wrote:
> > On Fri, Feb 5, 2021 at 6:45 PM Euler Taveira <euler@eulerto.com> wrote:
> >> - 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,
>
Thanks, but today again testing this API, I observed that we can still
get "tuple concurrently deleted" because we are releasing the lock on
ReplicationOriginRelationId at the end of API replorigin_drop_by_name.
So there is no guarantee that invalidation reaches other backend doing
the same operation. I think we need to keep the lock till the end of
xact as we do in other drop operations (see DropTableSpace, dropdb).
--
With Regards,
Amit Kapila.