Re: [HACKERS] Bug in StartupSUBTRANS

[Simon Riggs]

On 14 February 2016 at 00:03, Jeff Janes  wrote:


> I've attached a new version, incorporating comments from Tom and Michael.
>

Applied, thanks.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Bug in StartupSUBTRANS

[Michael Paquier]

On Sun, Feb 14, 2016 at 9:03 AM, Jeff Janes  wrote:
> I've repeated the crash/recovery/wrap-around testing with the proposed
> fix in place, and this pre-existing problem seems to be fixed now, and
> I have found no other problems.
>
> I've attached a new version, incorporating comments from Tom and Michael.

Thanks, this patch looks good to me this way.

I have added an entry in the CF app to not lose track of this bug:
https://commitfest.postgresql.org/9/526/
That's the most I can do.
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in StartupSUBTRANS

[Jeff Janes]

On Tue, Feb 9, 2016 at 10:33 AM, Simon Riggs  wrote:
> On 9 February 2016 at 18:42, Jeff Janes  wrote:
>>
>> While testing the crash resilience of the recent 2-part-commit
>> improvements, I've run into a problem where sometimes after a crash
>> the recovery process creates zeroed files in pg_subtrans until it
>> exhausts all disk space.
>
>
> Not sure which patch you're talking about there (2-part-commit).

The one here:

commit 978b2f65aa1262eb4ecbf8b3785cb1b9cf4db78e
Author: Simon Riggs 
Date:   Wed Jan 20 18:40:44 2016 -0800

Speedup 2PC by skipping two phase state files in normal path

I thought there was a second commit in the series, but I guess that is
still just proposed.  I've haven't tested that one, just the committed
one.

I've repeated the crash/recovery/wrap-around testing with the proposed
fix in place, and this pre-existing problem seems to be fixed now, and
I have found no other problems.

I've attached a new version, incorporating comments from Tom and Michael.

Cheers,

Jeff


StartupSUB_v2.patch
Description: Binary data

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in StartupSUBTRANS

[Tom Lane]

Simon Riggs  writes:
> Your patch looks right to me, so I will commit, barring objections... with
> backpatch. Likely to 9.0, AFAICS.

9.0 is out of support and should not be patched anymore.

I agree that the patch is basically correct, though I'd personally
write it without bothering with the extra variable:

+   /* must account for wraparound */
+   if (startPage > TransactionIdToPage(0x))
+   startPage = 0;

Also, the comment at line 45 is now wrong and needs an addition.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Re: [HACKERS] Bug in StartupSUBTRANS

[Simon Riggs]

On 9 February 2016 at 18:42, Jeff Janes  wrote:

> While testing the crash resilience of the recent 2-part-commit
> improvements, I've run into a problem where sometimes after a crash
> the recovery process creates zeroed files in pg_subtrans until it
> exhausts all disk space.
>

Not sure which patch you're talking about there (2-part-commit).


> Looking at the code, it looks like it does not anticipate that the xid
> might wrap around, meaning startPage/endPage might also wrap around.
> But obviously should not do so at int_max but rather at some much
> smaller other value.
>

Hmm, looks like the != part attempted to wrap, but just didn't get it right.

Your patch looks right to me, so I will commit, barring objections... with
backpatch. Likely to 9.0, AFAICS.

-- 
Simon Riggshttp://www.2ndQuadrant.com/

PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Bug in StartupSUBTRANS

[Michael Paquier]

On Wed, Feb 10, 2016 at 3:45 AM, Tom Lane  wrote:
> Simon Riggs  writes:
>> Your patch looks right to me, so I will commit, barring objections... with
>> backpatch. Likely to 9.0, AFAICS.
>
> 9.0 is out of support and should not be patched anymore.
>
> I agree that the patch is basically correct, though I'd personally
> write it without bothering with the extra variable:
>
> +   /* must account for wraparound */
> +   if (startPage > TransactionIdToPage(0x))
> +   startPage = 0;
>
> Also, the comment at line 45 is now wrong and needs an addition.

Instead of using a hardcoded value, wouldn't it be better to use
something based on MaxTransactionId?
-- 
Michael


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers