Re: Assertion failure in SnapBuildInitialSnapshot()

On Mon, Jan 30, 2023 at 8:24 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Thu, Dec 8, 2022 at 8:17 AM Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com> wrote:
> >
> > On Mon, Nov 21, 2022 at 4:31 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > One idea to fix this issue is that in
> > ReplicationSlotsComputeRequiredXmin(), we compute the minimum xmin
> > while holding both ProcArrayLock and ReplicationSlotControlLock, and
> > release only ReplicationSlotsControlLock before updating the
> > replication_slot_xmin. I'm concerned it will increase the contention
> > on ProcArrayLock but I've attached the patch for discussion.
> >
>
> But what kind of workload are you worried about? This will be called
> while processing XLOG_RUNNING_XACTS to update
> procArray->replication_slot_xmin/procArray->replication_slot_catalog_xmin
> only when required. So, if we want we can test some concurrent
> workloads along with walsenders doing the decoding to check if it
> impacts performance.
>

I was slightly concerned about holding ProcArrayLock while iterating
over replication slots especially when there are many replication
slots in the system. But you're right; we need it only when processing
XLOG_RUNINNG_XACTS and it's not frequent. So it doesn't introduce
visible overhead or negligible overhead.

> What other way we can fix this? Do you think we can try to avoid
> retreating xmin values in ProcArraySetReplicationSlotXmin() to avoid
> this problem? Personally, I think taking the lock as proposed by your
> patch is a better idea.

Agreed.

> BTW, this problem seems to be only logical
> replication specific, so if we are too worried then we can change this
> locking only for logical replication.

Yes, but I agree that there won't be a big overhead by this fix.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com