Re: Deadlock between logrep apply worker and tablesync worker - Mailing list pgsql-hackers

From vignesh C
Subject Re: Deadlock between logrep apply worker and tablesync worker
Date
Msg-id CALDaNm2_xXBSjccM_Wc57CdJ6XT7Zzdq32Qxt8B+4fQHK4Hyww@mail.gmail.com
Whole thread Raw
In response to RE: Deadlock between logrep apply worker and tablesync worker  ("houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>)
List pgsql-hackers
On Mon, 30 Jan 2023 at 13:00, houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> On Monday, January 30, 2023 2:32 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
> >
> > On Mon, Jan 30, 2023 at 9:20 AM vignesh C <vignesh21@gmail.com> wrote:
> > >
> > > On Sat, 28 Jan 2023 at 11:26, Amit Kapila <amit.kapila16@gmail.com> wrote:
> > > >
> > > > One thing that looks a bit odd is that we will anyway have a similar
> > > > check in replorigin_drop_guts() which is a static function and
> > > > called from only one place, so, will it be required to check at both places?
> > >
> > > There is a possibility that the initial check to verify if replication
> > > origin exists in replorigin_drop_by_name was successful but later one
> > > of either table sync worker or apply worker process might have dropped
> > > the replication origin,
> > >
> >
> > Won't locking on the particular origin prevent concurrent drops? IIUC, the
> > drop happens after the patch acquires the lock on the origin.
>
> Yes, I think the existence check in replorigin_drop_guts is unnecessary as we
> already lock the origin before that. I think the check in replorigin_drop_guts
> is a custom check after calling SearchSysCache1 to get the tuple, but the error
> should not happen as no concurrent drop can be performed.
>
> To make it simpler, one idea is to move the code that getting the tuple from
> system cache to the replorigin_drop_by_name(). After locking the origin, we
> can try to get the tuple and do the existence check, and we can reuse
> this tuple to perform origin delete. In this approach we only need to check
> origin existence once after locking. BTW, if we do this, then we'd better rename the
> replorigin_drop_guts() to something like replorigin_state_clear() as the function
> only clear the in-memory information after that.
>
> The code could be like:
>
> -------
> replorigin_drop_by_name(const char *name, bool missing_ok, bool nowait)
> ...
>         /*
>          * Lock the origin to prevent concurrent drops. We keep the lock until the
>          * end of transaction.
>          */
>         LockSharedObject(ReplicationOriginRelationId, roident, 0,
>                                          AccessExclusiveLock);
>
>         tuple = SearchSysCache1(REPLORIGIDENT, ObjectIdGetDatum(roident));
>         if (!HeapTupleIsValid(tuple))
>         {
>                 if (!missing_ok)
>                         elog(ERROR, "cache lookup failed for replication origin with ID %d",
>                                  roident);
>
>                 return;
>         }
>
>         replorigin_state_clear(rel, roident, nowait);
>
>         /*
>          * Now, we can delete the catalog entry.
>          */
>         CatalogTupleDelete(rel, &tuple->t_self);
>         ReleaseSysCache(tuple);
>
>         CommandCounterIncrement();
> ...

+1 for this change as it removes the redundant check which is not
required. I will post an updated version for this.

Regards,
Vignesh



pgsql-hackers by date:

Previous
From: John Naylor
Date:
Subject: Re: Considering additional sort specialisation functions for PG16
Next
From: "Takamichi Osumi (Fujitsu)"
Date:
Subject: RE: Time delayed LR (WAS Re: logical replication restrictions)