Re: BUGFIX: standby disconnect can corrupt serialized reorder buffers

From: Petr Jelinek <petr(dot)jelinek(at)2ndquadrant(dot)com>
To: Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>
Cc: Craig Ringer <craig(at)2ndquadrant(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)postgresql(dot)org>, Andres Freund <andres(at)anarazel(dot)de>
Subject: Re: BUGFIX: standby disconnect can corrupt serialized reorder buffers
Date: 2017-12-26 13:03:22
Message-ID: 066a6bc8-7be2-80a6-fb14-2a70536c02d1@2ndquadrant.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On 26/12/17 11:13, Masahiko Sawada wrote:
> On Tue, Dec 26, 2017 at 12:49 AM, Petr Jelinek
> <petr(dot)jelinek(at)2ndquadrant(dot)com> wrote:
>
>>>
>>> It's not a problem on crash restart because StartupReorderBuffer already
>>> does the required delete.
>>>
>>> ReorderBufferSerializeTXN, which spills the txns to disk, doesn't appear
>>> to have any guard to ensure that the segment files don't already exist
>>> when it goes to serialize a snapshot. Adding one there would probably be
>>> expensive; we don't know the last lsn of the txn yet, so to be really
>>> safe we'd have to do a directory listing and scan for any txn-$OURXID-*
>>> entries.
>>>
>>> So to fix, I suggest that we should do a
>>> slot-specific StartupReorderBuffer-style deletion when we start a new
>>> decoding session on a slot, per attached patch.
>>>
>>> It might be nice to also add a hook on proc exit, so we don't have stale
>>> buffers lying around until next decoding session, but I didn't want to
>>> add new global state to reorderbuffer.c so I've left that for now.
>>
>>
>> Hmm, can't we simply call the new cleanup function in
>> ReplicationSlotRelease()? That's called during process exit and we know
>> there about slot so no extra global variables are needed.
>>
>
> I guess that ReplicationSlotRelease() currently might not get called
> if walsender exits by proc_exit(). ReplicationSlotRelease() can is
> called by some functions such as WalSndErrorCleanup(), but at least in
> the case where wal sender exits due to failed to write data to socket,
> ReplicationSlotRelease() didn't get called as far as I tested.
>

Are you sure about that testing? Did you test it with replication slot
active? ReplicationSlotRelease() is called from ProcKill() if the
process is using a slot and should be called for any kind of exit except
for outright crash (the kind that makes postgres kill all backends). If
it wasn't we'd never unlock the replication slot used by the exiting
walsender.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2017-12-26 13:22:33 Re: [HACKERS] [PATCH] Lockable views
Previous Message Michael Paquier 2017-12-26 12:14:06 Re: Getting rid of "tuple concurrently updated" elog()s with concurrent DDLs (at least ALTER TABLE)