Re: [HACKERS] pg_serial early wraparound

2017-09-01 Thread Thomas Munro
On Wed, Jun 28, 2017 at 1:11 PM, Thomas Munro
 wrote:
> On Sat, Mar 25, 2017 at 7:27 AM, Thomas Munro
>  wrote:
>> On Sat, Mar 25, 2017 at 3:11 AM, Anastasia Lubennikova
>>  wrote:
>>> You claim that SLRUs now support five digit segment name, while in slru.h
>>> at current master I see the following:
>>>
>>>  * Note: slru.c currently assumes that segment file names will be four hex
>>>  * digits.  This sets a lower bound on the segment size (64K transactions
>>>  * for 32-bit TransactionIds).
>>>  */
>
> I've now complained about that comment in a separate thread.
>
>> It's not urgent, it's just cleanup work, so I've now moved it to the
>> next commitfest.  I will try to figure out a new way to demonstrate
>> that it works correctly without having to ask a review[er] to disable
>> any assertions.  Thanks again.

Rebased again, now with a commit message.  That assertion has since
been removed (commit ec99dd5a) so the attached test script can once
again be used to see the contents of pg_serial as the xid goes all the
way around, if you build with TEST_OLDSERXID defined so that
predicate.c forces information about xids out to pg_serial.

-- 
Thomas Munro
http://www.enterprisedb.com


ssi-slru-wraparound-v3.patch
Description: Binary data


ssi-slru-wraparound-test.sh
Description: Bourne shell script

-- 
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] pg_serial early wraparound

2017-06-27 Thread Thomas Munro
On Sat, Mar 25, 2017 at 7:27 AM, Thomas Munro
 wrote:
> On Sat, Mar 25, 2017 at 3:11 AM, Anastasia Lubennikova
>  wrote:
>> You claim that SLRUs now support five digit segment name, while in slru.h
>> at current master I see the following:
>>
>>  * Note: slru.c currently assumes that segment file names will be four hex
>>  * digits.  This sets a lower bound on the segment size (64K transactions
>>  * for 32-bit TransactionIds).
>>  */

I've now complained about that comment in a separate thread.

> It's not urgent, it's just cleanup work, so I've now moved it to the
> next commitfest.  I will try to figure out a new way to demonstrate
> that it works correctly without having to ask a review[er] to disable
> any assertions.  Thanks again.

Here's a rebased batch.

-- 
Thomas Munro
http://www.enterprisedb.com


ssi-slru-wraparound-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] pg_serial early wraparound

2017-03-24 Thread Thomas Munro
On Sat, Mar 25, 2017 at 3:11 AM, Anastasia Lubennikova
 wrote:
> Hi, I've tried to review this patch, but it seems that I miss something 
> essential.

Hi Anastasia,

Thanks for looking at this.

> You claim that SLRUs now support five digit segment name, while in slru.h
> at current master I see the following:
>
>  * Note: slru.c currently assumes that segment file names will be four hex
>  * digits.  This sets a lower bound on the segment size (64K transactions
>  * for 32-bit TransactionIds).
>  */
> #define SLRU_PAGES_PER_SEGMENT  32
>
> /* Maximum length of an SLRU name */
> #define SLRU_MAX_NAME_LENGTH32

That comment is out of date.  Commit 638cf09e extended SLRUs to
support 5 digit names, to support pg_multixact.  And I see now that
commit 73c986ad more recently created the possibility of 6 chacater
SLRU file names for pg_commit_ts.

> Could you please clarify the idea of the patch? Is it still relevant?

The idea is simply to remove some strange old code including scary
error messages that is no longer needed.  In my study of predicate.c
for other reasons, I noticed this in passing and thought I'd tidy it
up.  Because I have tangled with pg_multixact and seen 5-character
SLRU files with my own eyes, I knew that the restriction that
motivated this code was no longer valid.

> I've also run your test script.
> pg_clog was renamed to pg_xact, so it need to be changed accordingly
> echo "Contents of pg_clog:"
>   ls $PGDATA/pg_xact/

Right.

> The test shows failed assertion:
>
> == setting next xid to 1073741824 =
> Transaction log reset
> waiting for server to start2017-03-24 17:05:19.897 MSK [1181] LOG:  
> listening on IPv4 address "127.0.0.1", port 5432
> 2017-03-24 17:05:19.981 MSK [1181] LOG:  listening on Unix socket 
> "/tmp/.s.PGSQL.5432"
> 2017-03-24 17:05:20.081 MSK [1183] LOG:  database system was shut down at 
> 2017-03-24 17:05:19 MSK
> 2017-03-24 17:05:20.221 MSK [1181] LOG:  database system is ready to accept 
> connections
>  done
> server started
> vacuumdb: vacuuming database "postgres"
> vacuumdb: vacuuming database "template0"
> vacuumdb: vacuuming database "template1"
> TRAP: FailedAssertion("!(TransactionIdPrecedesOrEquals(oldestXact, 
> ShmemVariableCache->oldestXid))", File: "clog.c", Line: 669)
> vacuumdb: vacuuming of database "template1" failed: server closed the 
> connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> 2017-03-24 17:05:21.541 MSK [1181] LOG:  server process (PID 1202) was 
> terminated by signal 6: Aborted
> 2017-03-24 17:05:21.541 MSK [1181] DETAIL:  Failed process was running: 
> VACUUM (FREEZE);

My cheap trick for moving the xid around the clock quickly to test
wraparound scenarios no longer works, since this new assertion was
added in ea42cc18.  That was committed just a few hours before you
tested this.  Bad luck for me!

> The new status of this patch is: Waiting on Author

It's not urgent, it's just cleanup work, so I've now moved it to the
next commitfest.  I will try to figure out a new way to demonstrate
that it works correctly without having to ask a reviewing to disable
any assertions.  Thanks again.

-- 
Thomas Munro
http://www.enterprisedb.com


-- 
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] pg_serial early wraparound

2017-03-24 Thread Anastasia Lubennikova
The following review has been posted through the commitfest application:
make installcheck-world:  not tested
Implements feature:   not tested
Spec compliant:   not tested
Documentation:not tested

Hi, I've tried to review this patch, but it seems that I miss something 
essential.
You claim that SLRUs now support five digit segment name, while in slru.h
at current master I see the following:

 * Note: slru.c currently assumes that segment file names will be four hex
 * digits.  This sets a lower bound on the segment size (64K transactions
 * for 32-bit TransactionIds).
 */
#define SLRU_PAGES_PER_SEGMENT  32

/* Maximum length of an SLRU name */
#define SLRU_MAX_NAME_LENGTH32

Could you please clarify the idea of the patch? Is it still relevant?

I've also run your test script.
pg_clog was renamed to pg_xact, so it need to be changed accordingly
echo "Contents of pg_clog:"
  ls $PGDATA/pg_xact/


The test shows failed assertion:

== setting next xid to 1073741824 =
Transaction log reset
waiting for server to start2017-03-24 17:05:19.897 MSK [1181] LOG:  
listening on IPv4 address "127.0.0.1", port 5432
2017-03-24 17:05:19.981 MSK [1181] LOG:  listening on Unix socket 
"/tmp/.s.PGSQL.5432"
2017-03-24 17:05:20.081 MSK [1183] LOG:  database system was shut down at 
2017-03-24 17:05:19 MSK
2017-03-24 17:05:20.221 MSK [1181] LOG:  database system is ready to accept 
connections
 done
server started
vacuumdb: vacuuming database "postgres"
vacuumdb: vacuuming database "template0"
vacuumdb: vacuuming database "template1"
TRAP: FailedAssertion("!(TransactionIdPrecedesOrEquals(oldestXact, 
ShmemVariableCache->oldestXid))", File: "clog.c", Line: 669)
vacuumdb: vacuuming of database "template1" failed: server closed the 
connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
2017-03-24 17:05:21.541 MSK [1181] LOG:  server process (PID 1202) was 
terminated by signal 6: Aborted
2017-03-24 17:05:21.541 MSK [1181] DETAIL:  Failed process was running: VACUUM 
(FREEZE);

The new status of this patch is: Waiting on Author

-- 
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] pg_serial early wraparound

2017-02-26 Thread Thomas Munro
On Mon, Feb 27, 2017 at 7:28 PM, Thomas Munro
 wrote:
> On Wed, Nov 9, 2016 at 11:07 AM, Thomas Munro
>  wrote:
>> The SLRU managed by predicate.c can wrap around and overwrite data if
>> you have more than 1 billion active XIDs.  That's because when SSI was
>> implemented, slru.c was limited to four digit segment names, which
>> implied a page limit that wasn't enough for pg_serial to have space
>> for every possible XID.  We should probably rip that code out, because
>> SLRUs now support five digit segment names.  Something like the
>> attached.  I'll post a test script to demonstrate correct wraparound
>> behaviour around in time for one of the later CFs.
>
> Here is a shell script that shows a full rotation through xid space if
> you build PostgreSQL with TEST_OLDSERXID, which you can do by
> uncommenting a line in predicate.c.  On master we see the SLRU
> segments go around the clock twice for each time xid goes around.
> With the patch it goes around just once, adding an extra character to
> the segment name to double the space.

I attached the wrong version.  Here is the right one.

-- 
Thomas Munro
http://www.enterprisedb.com


ssi-slru-wraparound-test.sh
Description: Bourne shell script

-- 
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] pg_serial early wraparound

2017-02-26 Thread Thomas Munro
On Wed, Nov 9, 2016 at 11:07 AM, Thomas Munro
 wrote:
> The SLRU managed by predicate.c can wrap around and overwrite data if
> you have more than 1 billion active XIDs.  That's because when SSI was
> implemented, slru.c was limited to four digit segment names, which
> implied a page limit that wasn't enough for pg_serial to have space
> for every possible XID.  We should probably rip that code out, because
> SLRUs now support five digit segment names.  Something like the
> attached.  I'll post a test script to demonstrate correct wraparound
> behaviour around in time for one of the later CFs.

Here is a shell script that shows a full rotation through xid space if
you build PostgreSQL with TEST_OLDSERXID, which you can do by
uncommenting a line in predicate.c.  On master we see the SLRU
segments go around the clock twice for each time xid goes around.
With the patch it goes around just once, adding an extra character to
the segment name to double the space.

By the way, I think the real number of xids it can hold today is
(65536 * 32 * 8192) / sizeof(uint64) = 2^31 xids, not 2^30 as
indicated by an existing comment.  So I think there is actually enough
space and wrapping is probably harmess, but it seems cleaner and
simpler not to do that and to rip out the scary warning code, so I'll
add this to the CF.

-- 
Thomas Munro
http://www.enterprisedb.com


ssi-slru-wraparound-test.sh
Description: Bourne shell script

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


[HACKERS] pg_serial early wraparound

2016-11-08 Thread Thomas Munro
Hi hackers,

The SLRU managed by predicate.c can wrap around and overwrite data if
you have more than 1 billion active XIDs.  That's because when SSI was
implemented, slru.c was limited to four digit segment names, which
implied a page limit that wasn't enough for pg_serial to have space
for every possible XID.  We should probably rip that code out, because
SLRUs now support five digit segment names.  Something like the
attached.  I'll post a test script to demonstrate correct wraparound
behaviour around in time for one of the later CFs.

-- 
Thomas Munro
http://www.enterprisedb.com


ssi-slru-wraparound-v1.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