Horton

Thread

Commits

  1. Acquire spinlock when updating 2PC slot data during logical decoding creation

  2. Add support for prepared transactions to built-in logical replication.

  1. Spinlock is missing when updating two_phase of ReplicationSlot

    Masahiko Sawada <sawada.mshk@gmail.com> — 2023-01-11T02:07:05Z

    Hi,

I realized that in CreateDecodingContext() function, we update both
slot->data.two_phase and two_phase_at without acquiring the spinlock:

    /* Mark slot to allow two_phase decoding if not already marked */
    if (ctx->twophase && !slot->data.two_phase)
    {
        slot->data.two_phase = true;
        slot->data.two_phase_at = start_lsn;
        ReplicationSlotMarkDirty();
        ReplicationSlotSave();
        SnapBuildSetTwoPhaseAt(ctx->snapshot_builder, start_lsn);
    }

I think we should acquire the spinlock when updating fields of the
replication slot even by its owner. Otherwise readers could see
inconsistent results. Looking at another place where we update
two_phase_at, we acquire the spinlock:

    SpinLockAcquire(&slot->mutex);
    slot->data.confirmed_flush = ctx->reader->EndRecPtr;
    if (slot->data.two_phase)
        slot->data.two_phase_at = ctx->reader->EndRecPtr;
    SpinLockRelease(&slot->mutex);

It seems to me an oversight of commit a8fd13cab0b. I've attached the
small patch to fix it.

Regards,

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

  2. Re: Spinlock is missing when updating two_phase of ReplicationSlot

    Michael Paquier <michael@paquier.xyz> — 2023-01-11T05:36:17Z

    On Wed, Jan 11, 2023 at 11:07:05AM +0900, Masahiko Sawada wrote:
> I think we should acquire the spinlock when updating fields of the
> replication slot even by its owner. Otherwise readers could see
> inconsistent results. Looking at another place where we update
> two_phase_at, we acquire the spinlock:
> 
>     SpinLockAcquire(&slot->mutex);
>     slot->data.confirmed_flush = ctx->reader->EndRecPtr;
>     if (slot->data.two_phase)
>         slot->data.two_phase_at = ctx->reader->EndRecPtr;
>     SpinLockRelease(&slot->mutex);
> 
> It seems to me an oversight of commit a8fd13cab0b. I've attached the
> small patch to fix it.

Looks right to me, the paths updating the data related to the slots
are careful about that, even when it comes to fetching a slot from
MyReplicationSlot.  I have been looking around the slot code to see if
there are other inconsistencies, and did not notice anything standing
out.  Will fix..
--
Michael

  3. Re: Spinlock is missing when updating two_phase of ReplicationSlot

    Michael Paquier <michael@paquier.xyz> — 2023-01-12T04:42:11Z

    On Wed, Jan 11, 2023 at 02:36:17PM +0900, Michael Paquier wrote:
> Looks right to me, the paths updating the data related to the slots
> are careful about that, even when it comes to fetching a slot from
> MyReplicationSlot.  I have been looking around the slot code to see if
> there are other inconsistencies, and did not notice anything standing
> out.  Will fix..

And done, thanks!
--
Michael

  4. Re: Spinlock is missing when updating two_phase of ReplicationSlot

    Masahiko Sawada <sawada.mshk@gmail.com> — 2023-01-12T05:05:44Z

    On Thu, Jan 12, 2023 at 1:42 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Jan 11, 2023 at 02:36:17PM +0900, Michael Paquier wrote:
> > Looks right to me, the paths updating the data related to the slots
> > are careful about that, even when it comes to fetching a slot from
> > MyReplicationSlot.  I have been looking around the slot code to see if
> > there are other inconsistencies, and did not notice anything standing
> > out.  Will fix..
>
> And done, thanks!

Thank you!

Regards,

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