Horton

Thread

Commits

  1. Avoid repeating loads of frozen ID values.

  2. Close race condition between datfrozen and relfrozen updates.

  3. Fetch XIDs atomically during vac_truncate_clog().

  1. Autovacuum to prevent wraparound tries to consume xid

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2016-03-28T11:05:12Z

    Hackers,

one our customer meet near xid wraparound situation.  xid counter
reached xidStopLimit value.  So, no transactions could be executed in
normal mode.  But what I noticed is strange behaviour of autovacuum to
prevent wraparound.  It vacuums tables, updates pg_class and pg_database,
but then falls with "database is not accepting commands to avoid wraparound
data loss in database" message.  We end up with situation that according to
pg_database  maximum age of database was less than 200 mln., but
transactions couldn't be executed, because ShmemVariableCache wasn't
updated (checked by gdb).

I've reproduced this situation on my laptop as following:

1) Connect gdb, do "set ShmemVariableCache->nextXid =
ShmemVariableCache->xidStopLimit"
2) Stop postgres
3) Make some fake clog: "dd bs=1m if=/dev/zero
of=/usr/local/pgsql/data/pg_clog/07FF count=1024"
4) Start postgres

Then I found the same situation as in customer database.  Autovacuum to
prevent wraparound regularly produced following messages in the log:

ERROR:  database is not accepting commands to avoid wraparound data loss in
database "template1"
HINT:  Stop the postmaster and vacuum that database in single-user mode.
You might also need to commit or roll back old prepared transactions.

Finally all databases was frozen

# SELECT datname, age(datfrozenxid) FROM pg_database;
  datname  │   age
───────────┼──────────
 template1 │        0
 template0 │        0
 postgres  │ 50000000
(3 rows)

but no transactions could be executed (ShmemVariableCache wasn't updated).

After some debugging I found that vac_truncate_clog consumes xid just to
produce warning.  I wrote simple patch which replaces
GetCurrentTransactionId() with ShmemVariableCache->nextXid.  That
completely fixes this situation for me: ShmemVariableCache was successfully
updated.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  2. Re: Autovacuum to prevent wraparound tries to consume xid

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2016-05-19T14:14:31Z

    On Mon, Mar 28, 2016 at 2:05 PM, Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:

> After some debugging I found that vac_truncate_clog consumes xid just to
> produce warning.  I wrote simple patch which replaces
> GetCurrentTransactionId() with ShmemVariableCache->nextXid.  That
> completely fixes this situation for me: ShmemVariableCache was successfully
> updated.
>

I found that direct reading of ShmemVariableCache->nextXid is not corrent,
it's better to use ReadNewTransactionId() then.  Fixed version of patch is
attached.

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  3. Re: Autovacuum to prevent wraparound tries to consume xid

    Amit Kapila <amit.kapila16@gmail.com> — 2016-05-22T09:39:05Z

    On Mon, Mar 28, 2016 at 4:35 PM, Alexander Korotkov <
a.korotkov@postgrespro.ru> wrote:

> Hackers,
>
> one our customer meet near xid wraparound situation.  xid counter
> reached xidStopLimit value.  So, no transactions could be executed in
> normal mode.  But what I noticed is strange behaviour of autovacuum to
> prevent wraparound.  It vacuums tables, updates pg_class and pg_database,
> but then falls with "database is not accepting commands to avoid wraparound
> data loss in database" message.  We end up with situation that according to
> pg_database  maximum age of database was less than 200 mln., but
> transactions couldn't be executed, because ShmemVariableCache wasn't
> updated (checked by gdb).
>
> I've reproduced this situation on my laptop as following:
>
> 1) Connect gdb, do "set ShmemVariableCache->nextXid =
> ShmemVariableCache->xidStopLimit"
> 2) Stop postgres
> 3) Make some fake clog: "dd bs=1m if=/dev/zero
> of=/usr/local/pgsql/data/pg_clog/07FF count=1024"
> 4) Start postgres
>
> Then I found the same situation as in customer database.  Autovacuum to
> prevent wraparound regularly produced following messages in the log:
>
> ERROR:  database is not accepting commands to avoid wraparound data loss
> in database "template1"
> HINT:  Stop the postmaster and vacuum that database in single-user mode.
> You might also need to commit or roll back old prepared transactions.
>
> Finally all databases was frozen
>
> # SELECT datname, age(datfrozenxid) FROM pg_database;
>   datname  │   age
> ───────────┼──────────
>  template1 │        0
>  template0 │        0
>  postgres  │ 50000000
> (3 rows)
>
> but no transactions could be executed (ShmemVariableCache wasn't updated).
>
> After some debugging I found that vac_truncate_clog consumes xid just to
> produce warning.  I wrote simple patch which replaces
> GetCurrentTransactionId() with ShmemVariableCache->nextXid.  That
> completely fixes this situation for me: ShmemVariableCache was successfully
> updated.
>

As per your latest patch, you are using ReadNewTransactionId() to get the
nextXid which then is used to check if any database's frozenxid is already
wrapped.  Now, isn't the value of nextXID in your patch same as
lastSaneFrozenXid in most cases (I mean there is a small window where some
new transaction might have started due to which the value of
ShmemVariableCache->nextXid has been advanced)? So isn't relying on
lastSaneFrozenXid check sufficient?


With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com

  4. Re: Autovacuum to prevent wraparound tries to consume xid

    Alexander Korotkov <a.korotkov@postgrespro.ru> — 2016-05-22T12:24:04Z

    On Sun, May 22, 2016 at 12:39 PM, Amit Kapila <amit.kapila16@gmail.com>
wrote:

> On Mon, Mar 28, 2016 at 4:35 PM, Alexander Korotkov <
> a.korotkov@postgrespro.ru> wrote:
>
>> Hackers,
>>
>> one our customer meet near xid wraparound situation.  xid counter
>> reached xidStopLimit value.  So, no transactions could be executed in
>> normal mode.  But what I noticed is strange behaviour of autovacuum to
>> prevent wraparound.  It vacuums tables, updates pg_class and pg_database,
>> but then falls with "database is not accepting commands to avoid wraparound
>> data loss in database" message.  We end up with situation that according to
>> pg_database  maximum age of database was less than 200 mln., but
>> transactions couldn't be executed, because ShmemVariableCache wasn't
>> updated (checked by gdb).
>>
>> I've reproduced this situation on my laptop as following:
>>
>> 1) Connect gdb, do "set ShmemVariableCache->nextXid =
>> ShmemVariableCache->xidStopLimit"
>> 2) Stop postgres
>> 3) Make some fake clog: "dd bs=1m if=/dev/zero
>> of=/usr/local/pgsql/data/pg_clog/07FF count=1024"
>> 4) Start postgres
>>
>> Then I found the same situation as in customer database.  Autovacuum to
>> prevent wraparound regularly produced following messages in the log:
>>
>> ERROR:  database is not accepting commands to avoid wraparound data loss
>> in database "template1"
>> HINT:  Stop the postmaster and vacuum that database in single-user mode.
>> You might also need to commit or roll back old prepared transactions.
>>
>> Finally all databases was frozen
>>
>> # SELECT datname, age(datfrozenxid) FROM pg_database;
>>   datname  │   age
>> ───────────┼──────────
>>  template1 │        0
>>  template0 │        0
>>  postgres  │ 50000000
>> (3 rows)
>>
>> but no transactions could be executed (ShmemVariableCache wasn't updated).
>>
>> After some debugging I found that vac_truncate_clog consumes xid just to
>> produce warning.  I wrote simple patch which replaces
>> GetCurrentTransactionId() with ShmemVariableCache->nextXid.  That
>> completely fixes this situation for me: ShmemVariableCache was successfully
>> updated.
>>
>
> As per your latest patch, you are using ReadNewTransactionId() to get the
> nextXid which then is used to check if any database's frozenxid is already
> wrapped.  Now, isn't the value of nextXID in your patch same as
> lastSaneFrozenXid in most cases (I mean there is a small window where some
> new transaction might have started due to which the value of
> ShmemVariableCache->nextXid has been advanced)? So isn't relying on
> lastSaneFrozenXid check sufficient?
>

Hmm... So, this code already contains comparison with lastSaneFrozenXid.
Thus, current code compares against both of lastSaneFrozenXid and myXID.  I
have no comment clarifying why this should be so.  In my opinion we can
just remove myXID with its checks.  Git shows that Tom Lane
committed lastSaneFrozenXid and lastSaneMinMulti checks in addition to
myXID check in 78db307b.

Tom, what do you think?  Could we remove myXID from vac_truncate_clog()?

------
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

  5. Re: Autovacuum to prevent wraparound tries to consume xid

    Tom Lane <tgl@sss.pgh.pa.us> — 2016-05-24T19:01:13Z

    Alexander Korotkov <a.korotkov@postgrespro.ru> writes:
> On Sun, May 22, 2016 at 12:39 PM, Amit Kapila <amit.kapila16@gmail.com>
> wrote:
>> As per your latest patch, you are using ReadNewTransactionId() to get the
>> nextXid which then is used to check if any database's frozenxid is already
>> wrapped.  Now, isn't the value of nextXID in your patch same as
>> lastSaneFrozenXid in most cases (I mean there is a small window where some
>> new transaction might have started due to which the value of
>> ShmemVariableCache->nextXid has been advanced)? So isn't relying on
>> lastSaneFrozenXid check sufficient?

> Hmm... So, this code already contains comparison with lastSaneFrozenXid.
> Thus, current code compares against both of lastSaneFrozenXid and myXID.  I
> have no comment clarifying why this should be so.  In my opinion we can
> just remove myXID with its checks.  Git shows that Tom Lane
> committed lastSaneFrozenXid and lastSaneMinMulti checks in addition to
> myXID check in 78db307b.

> Tom, what do you think?  Could we remove myXID from vac_truncate_clog()?

Well, I do not want to remove the frozenAlreadyWrapped check.  In
principle that can't happen anymore ... but if it did, truncating CLOG
would be a really bad response.

I don't see any good reason for this function to assume that
lastSaneFrozenXid is indistinguishably close to the current latest XID.
That would be introducing a not-very-obvious dependency on how the caller
computes that value, which is the same kind of context dependency that
caused this bug in the first place --- I'm pretty sure that this code was
fine when written, because it couldn't be invoked in a transaction without
an XID.

Checking against ReadNewTransactionId() is actually better than what's
there now, since it's guaranteed to be the upper limit of XIDs in use,
which the transaction's own XID certainly isn't.

So I think Alexander's revised patch is good as-is.  Will push in a bit.

Also, I notice another problem in vac_truncate_clog() now that I'm looking
at it: it's expecting that the pg_database datfrozenxid and datminmxid
values will hold still while it's looking at them.  Since
vac_update_datfrozenxid updates those values in-place, there's a race
condition against VACUUMs happening in other databases.  We should fetch
those values into local variables before doing the various tests inside
the scan loop.

			regards, tom lane

  6. datfrozen/relfrozen update race condition

    Noah Misch <noah@leadboat.com> — 2024-04-23T00:39:56Z

    On Tue, May 24, 2016 at 03:01:13PM -0400, Tom Lane wrote:
> Also, I notice another problem in vac_truncate_clog() now that I'm looking
> at it: it's expecting that the pg_database datfrozenxid and datminmxid
> values will hold still while it's looking at them.  Since
> vac_update_datfrozenxid updates those values in-place, there's a race
> condition against VACUUMs happening in other databases.  We should fetch
> those values into local variables before doing the various tests inside
> the scan loop.

Commit 2d2e40e fixed the above.  There's another problem just like it, one
layer lower.  vac_update_datfrozenxid() has:

			if (TransactionIdPrecedes(classForm->relfrozenxid, newFrozenXid))
				newFrozenXid = classForm->relfrozenxid;

classForm points to buffer memory, and vac_update_relstats() inplace-updates
the buffer.  Like vac_truncate_clog(), we don't mind using an old value, but
those two lines must use the same value.  The attached test case shows this
bug making datfrozenxid move ahead of relfrozenxid.  The attached patch fixes
it.  (I noticed this while finishing up patches for the heap_inplace_update
writer race in https://postgr.es/m/20231102030915.d3.nmisch@google.com.)

I audited other read-only use of inplace-updated fields.  Others look safe,
because they hold rel locks that exclude VACUUM, or they make only
non-critical decisions.  Still, let's change some to the load-once style, to
improve the chance of future copy/paste finding the safe style.  I'm attaching
a patch for that, too.  I didn't add "volatile", because I couldn't think of
how we'd care if the load moved earlier.