Thread: [HACKERS] Failed recovery with new faster 2PC code

On 15 April 2017 at 23:37, Jeff Janes <jeff.janes@gmail.com> wrote:
> After this commit, I get crash recovery failures when using prepared
> transactions.
>
> commit 728bd991c3c4389fb39c45dcb0fe57e4a1dccd71
> Author: Simon Riggs <simon@2ndQuadrant.com>
> Date:   Tue Apr 4 15:56:56 2017 -0400
>
>     Speedup 2PC recovery by skipping two phase state files in normal path

Thanks Jeff for your tests.

So that's now two crash bugs in as many days and lack of clarity about
how to fix it.

Stas, I thought this patch was very important to you, yet two releases
in a row we are too-late-and-buggy.

If anybody has a reason why I shouldn't revert this, please say so now
fairly soon.
Any further attempts to fix must run Jeff's tests.

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> On 17 Apr 2017, at 12:14, Simon Riggs <simon@2ndquadrant.com> wrote:
>
> On 15 April 2017 at 23:37, Jeff Janes <jeff.janes@gmail.com> wrote:
>> After this commit, I get crash recovery failures when using prepared
>> transactions.
>>
>> commit 728bd991c3c4389fb39c45dcb0fe57e4a1dccd71
>> Author: Simon Riggs <simon@2ndQuadrant.com>
>> Date:   Tue Apr 4 15:56:56 2017 -0400
>>
>>    Speedup 2PC recovery by skipping two phase state files in normal path
>
> Thanks Jeff for your tests.
>
> So that's now two crash bugs in as many days and lack of clarity about
> how to fix it.
>
> Stas, I thought this patch was very important to you, yet two releases
> in a row we are too-late-and-buggy.

I’m looking at pgstat issue in nearby thread right now and will switch to this
shortly.

If that’s possible, I’m asking to delay revert for several days.

Stas Kelvich
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On 18 April 2017 at 09:57, Nikhil Sontakke <nikhils@2ndquadrant.com> wrote:

> Please find attached a second version of my bug fix which is stylistically
> better and clearer than the first one.

Yeh, this is better. Pushed.


The bug was that the loop set gxact to be the last entry in the array,
causing the exit condition to fail and us then to remove the last
gxact from memory even when it didn't match the xid, removing a valid
entry too early. That then allowed xmin to move forwards, which causes
autovac to remove pg_xact entries earlier than needed.

Well done for finding that one, thanks for the patch.

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Apr 18, 2017 at 7:54 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
> Yeh, this is better. Pushed.

I have been outraced on this one, the error is obvious once you see it ;)

Thanks for the investigation and the fix! I have spent a couple of
hours reviewing the interactions between the shmem entries of 2PC
state data created at the beginning of recovery and all the lookups in
procarray.c and varsup.c, noticing nothing by the way.

> The bug was that the loop set gxact to be the last entry in the array,
> causing the exit condition to fail and us then to remove the last
> gxact from memory even when it didn't match the xid, removing a valid
> entry too early. That then allowed xmin to move forwards, which causes
> autovac to remove pg_xact entries earlier than needed.
>
> Well done for finding that one, thanks for the patch.

Running Jeff's test suite, I can confirm that there are no problems now.
-- 
Michael

On 18 April 2017 at 13:12, Michael Paquier <michael.paquier@gmail.com> wrote:
> On Tue, Apr 18, 2017 at 7:54 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
>> Yeh, this is better. Pushed.
>
> I have been outraced on this one, the error is obvious once you see it ;)

Didn't realise you were working on it, nothing competitive about it.

It's clear this needed fixing, whether or not it fixes Jeff's report.

I do think it explains the report, so I'm hopeful.

-- 
Simon Riggs                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Apr 19, 2017 at 11:09 AM, Jeff Janes <jeff.janes@gmail.com> wrote:
> Would this bug have been seen in a replica server in the absence of crashes,
> or was it only vulnerable during crash recovery rather than streaming
> replication?

This issue could have been seen on a streaming standby as well,
letting around a TwoPhaseState impacts as well their linked PGPROC so
CLOG truncation would have been messed up as well. That's also the
case of the first issue involving as well incorrect XID updates,
though the chances to see it are I think lower as only the beginning
of recovery was impacted.
-- 
Michael