Re: Bug in two-phase transaction recovery

On 8 September 2016 at 07:43, Michael Paquier <michael(dot)paquier(at)gmail(dot)com> wrote:
> On Wed, Sep 7, 2016 at 10:48 PM, Stas Kelvich <s(dot)kelvich(at)postgrespro(dot)ru> wrote:
>> Some time ago two-phase state file format was changed to have variable size GID,
>> but several places that read that files were not updated to use new offsets. Problem
>> exists in master and 9.6 and can be reproduced on prepared transactions with
>> savepoints.
>
> Oops and meh. This meritates an open item, and has better be fixed by
> 9.6.0. I am glad you noticed that honestly. And we had better take
> care of this issue as soon as possible.

Looking now.

>> Also while looking at StandbyRecoverPreparedTransactions() i’ve noticed that buffer
>> for 2pc file is allocated in TopMemoryContext but never freed. That probably exists
>> for a long time.
>
> @@ -1886,6 +1886,8 @@ StandbyRecoverPreparedTransactions(bool overwriteOK)
> Assert(TransactionIdFollows(subxid, xid));
> SubTransSetParent(xid, subxid, overwriteOK);
> }
> +
> + pfree(buf);
> }
> This one is a good catch. I have checked also the other callers of
> ReadTwoPhaseFile but I am not seeing any other leak. That's a leak,
> not critical though so applying it only on HEAD would be enough IMO.

Far from critical, but backpatched to 9.6 because it isn't just
executed once at startup, it is executed every shutdown checkpoint.

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