Not HOT enough
Some time ago, I changed GetOldestXmin() to ignore procs in other
databases resulting in a potentially later xmin.
GetSnapshotData() was not touched when that happened, even though the
comments say "...This is the same computation done by
GetOldestXmin(true, true)." The transam/README file says it stronger
"GetSnapshotData also performs an oldest-xmin calculation (which had
better
match GetOldestXmin's)". Doh.
As a result, VACUUM ignores procs in other databases, whereas HOT does
not. That means we aren't cleaning up as much as we could do when
running multiple databases. If its OK for VACUUM, then it must be OK
for HOT cleanup also.
Attached patch ignores procs in other databases during
GetSnapshotData() when IsMVCCSnapshot(), using similar coding to
GetOldestXmin().
Any doubters?
I suggest this is backpatched a few releases.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
fix_getsnapshotdata.v1.patchapplication/octet-stream; name=fix_getsnapshotdata.v1.patchDownload+8-0
On Tue, Nov 22, 2011 at 3:23 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
Some time ago, I changed GetOldestXmin() to ignore procs in other
databases resulting in a potentially later xmin.GetSnapshotData() was not touched when that happened, even though the
comments say "...This is the same computation done by
GetOldestXmin(true, true)." The transam/README file says it stronger
"GetSnapshotData also performs an oldest-xmin calculation (which had
better
match GetOldestXmin's)". Doh.As a result, VACUUM ignores procs in other databases, whereas HOT does
not. That means we aren't cleaning up as much as we could do when
running multiple databases. If its OK for VACUUM, then it must be OK
for HOT cleanup also.Attached patch ignores procs in other databases during
GetSnapshotData() when IsMVCCSnapshot(), using similar coding to
GetOldestXmin().Any doubters?
I think this is unsafe for shared catalogs.
I suggest this is backpatched a few releases.
-1.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Tue, Nov 22, 2011 at 9:21 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Nov 22, 2011 at 3:23 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
Some time ago, I changed GetOldestXmin() to ignore procs in other
databases resulting in a potentially later xmin.GetSnapshotData() was not touched when that happened, even though the
comments say "...This is the same computation done by
GetOldestXmin(true, true)." The transam/README file says it stronger
"GetSnapshotData also performs an oldest-xmin calculation (which had
better
match GetOldestXmin's)". Doh.As a result, VACUUM ignores procs in other databases, whereas HOT does
not. That means we aren't cleaning up as much as we could do when
running multiple databases. If its OK for VACUUM, then it must be OK
for HOT cleanup also.Attached patch ignores procs in other databases during
GetSnapshotData() when IsMVCCSnapshot(), using similar coding to
GetOldestXmin().Any doubters?
I think this is unsafe for shared catalogs.
I think so too. Thats why it uses IsMVCCSnapshot() to confirm when it
is safe to do so.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Tue, Nov 22, 2011 at 11:40 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
I think this is unsafe for shared catalogs.
I think so too. Thats why it uses IsMVCCSnapshot() to confirm when it
is safe to do so.
Ah, you mean access to shared catalogs using MVCC snapshots.
Fixed.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
fix_getsnapshotdata.v2.patchapplication/octet-stream; name=fix_getsnapshotdata.v2.patchDownload+16-1
On Tue, Nov 22, 2011 at 7:25 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
On Tue, Nov 22, 2011 at 11:40 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
I think this is unsafe for shared catalogs.
I think so too. Thats why it uses IsMVCCSnapshot() to confirm when it
is safe to do so.Ah, you mean access to shared catalogs using MVCC snapshots.
Yeah. This change would have the disadvantage of disabling HOT
cleanup for shared catalogs; I'm not sure whether that's a good
decision.
But now that you mention it, something seems funky about the other bit
you mention, too:
+ /* MVCC snapshots ignore other databases */
+ if (!allDbs &&
+ proc->databaseId != MyDatabaseId &&
+ proc->databaseId != 0) /* always include WalSender */
+ continue;
+It doesn't make sense for the RecentGlobalXmin calculation to depend
on whether or not the current snapshot is an MVCC snapshot, because
RecentGlobalXmin is a global variable not related to any particular
snapshot. I don't believe it's safe to assume that RecentGlobalXmin
will only ever be used in conjunction with the most-recently-taken
snapshot.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Nov 23, 2011 at 2:00 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Nov 22, 2011 at 7:25 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
On Tue, Nov 22, 2011 at 11:40 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
I think this is unsafe for shared catalogs.
I think so too. Thats why it uses IsMVCCSnapshot() to confirm when it
is safe to do so.Ah, you mean access to shared catalogs using MVCC snapshots.
Yeah. This change would have the disadvantage of disabling HOT
cleanup for shared catalogs; I'm not sure whether that's a good
decision.
No, it disables cleanup when being read. They are still VACUUMed normally.
Note that non-MVCC snapshots never did run HOT page-level cleanup, so
this hardly changes anything.
And it effects shared catalogs only, which are all low traffic anyway.
But now that you mention it, something seems funky about the other bit
you mention, too:+ /* MVCC snapshots ignore other databases */ + if (!allDbs && + proc->databaseId != MyDatabaseId && + proc->databaseId != 0) /* always include WalSender */ + continue; +It doesn't make sense for the RecentGlobalXmin calculation to depend
on whether or not the current snapshot is an MVCC snapshot, because
RecentGlobalXmin is a global variable not related to any particular
snapshot. I don't believe it's safe to assume that RecentGlobalXmin
will only ever be used in conjunction with the most-recently-taken
snapshot.
Why would that matter exactly? RecentGlobalXmin is used in 4 places
and this works with them all.
This changes the meaning of that variable from what it was previously,
but so what? It's backend local.
The huge benefit is that we clean up data in normal tables much better
than we did before in cases where people use multiple databases, which
is a common case.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
On Wed, Nov 23, 2011 at 9:25 AM, Simon Riggs <simon@2ndquadrant.com> wrote:
Yeah. This change would have the disadvantage of disabling HOT
cleanup for shared catalogs; I'm not sure whether that's a good
decision.No, it disables cleanup when being read. They are still VACUUMed normally.
Note that non-MVCC snapshots never did run HOT page-level cleanup, so
this hardly changes anything.And it effects shared catalogs only, which are all low traffic anyway.
I think "low traffic" is the key point. I understand that you're not
changing the VACUUM behavior, but you are making heap_page_prune_opt()
not do anything when a shared catalog is involved. That would be
unacceptable if we expected shared catalogs to be updated frequently,
either now or in the future, but I guess we don't expect that.
I suppose we could compute two RecentGlobalXmin values, one for all
databases and one for just the current database. But that would make
GetSnapshotData() slower, which would almost certainly be a cure worse
than the disease.
But now that you mention it, something seems funky about the other bit
you mention, too:+ /* MVCC snapshots ignore other databases */ + if (!allDbs && + proc->databaseId != MyDatabaseId && + proc->databaseId != 0) /* always include WalSender */ + continue; +It doesn't make sense for the RecentGlobalXmin calculation to depend
on whether or not the current snapshot is an MVCC snapshot, because
RecentGlobalXmin is a global variable not related to any particular
snapshot. I don't believe it's safe to assume that RecentGlobalXmin
will only ever be used in conjunction with the most-recently-taken
snapshot.Why would that matter exactly? RecentGlobalXmin is used in 4 places
and this works with them all.This changes the meaning of that variable from what it was previously,
but so what? It's backend local.
I don't object to changing the meaning of the variable, but I don't
understand how it can be correct to include backends from other
databases in the RecentGlobalXmin calculation when using an MVCC
snapshot, but not otherwise. Come to think of it, when does
GetSnapshotData() get called with a non-MVCC snapshot anyway?
The huge benefit is that we clean up data in normal tables much better
than we did before in cases where people use multiple databases, which
is a common case.
I understand the benefit; I just want to make sure we're not going to
break anything.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of mié nov 23 12:15:55 -0300 2011:
And it effects shared catalogs only, which are all low traffic anyway.
I think "low traffic" is the key point. I understand that you're not
changing the VACUUM behavior, but you are making heap_page_prune_opt()
not do anything when a shared catalog is involved. That would be
unacceptable if we expected shared catalogs to be updated frequently,
either now or in the future, but I guess we don't expect that.
Maybe not pg_database or pg_tablespace and such, but I'm not so sure
about pg_shdepend. (Do we record pg_shdepend entries for temp tables?)
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Wed, Nov 23, 2011 at 10:20 AM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
Excerpts from Robert Haas's message of mié nov 23 12:15:55 -0300 2011:
And it effects shared catalogs only, which are all low traffic anyway.
I think "low traffic" is the key point. I understand that you're not
changing the VACUUM behavior, but you are making heap_page_prune_opt()
not do anything when a shared catalog is involved. That would be
unacceptable if we expected shared catalogs to be updated frequently,
either now or in the future, but I guess we don't expect that.Maybe not pg_database or pg_tablespace and such, but I'm not so sure
about pg_shdepend. (Do we record pg_shdepend entries for temp tables?)
Hmm, I'm not seeing any increase in the number of entries in
pg_shdepend when I create either a temporary or permanent table:
rhaas=# select sum(1) from pg_shdepend;
sum
-----
2
(1 row)
rhaas=# create temp table xyz (a int);
CREATE TABLE
rhaas=# select sum(1) from pg_shdepend;
sum
-----
2
(1 row)
rhaas=# create table abc (a int);
CREATE TABLE
rhaas=# select sum(1) from pg_shdepend;
sum
-----
2
(1 row)
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
Excerpts from Robert Haas's message of mié nov 23 12:28:38 -0300 2011:
Hmm, I'm not seeing any increase in the number of entries in
pg_shdepend when I create either a temporary or permanent table:rhaas=# select sum(1) from pg_shdepend;
sum
-----
2
(1 row)rhaas=# create temp table xyz (a int);
CREATE TABLE
rhaas=# select sum(1) from pg_shdepend;
sum
-----
2
(1 row)
That's because the owner is "pinned" (i.e. the bootstrap user). Try
with a different user. I see new rows with both temp and non-temp
tables.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
On Wed, Nov 23, 2011 at 10:35 AM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
That's because the owner is "pinned" (i.e. the bootstrap user). Try
with a different user. I see new rows with both temp and non-temp
tables.
Oh, wow. I had no idea it worked like that. You learn something new every day.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
On Wed, Nov 23, 2011 at 3:20 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:
Excerpts from Robert Haas's message of mié nov 23 12:15:55 -0300 2011:
And it effects shared catalogs only, which are all low traffic anyway.
I think "low traffic" is the key point. I understand that you're not
changing the VACUUM behavior, but you are making heap_page_prune_opt()
not do anything when a shared catalog is involved. That would be
unacceptable if we expected shared catalogs to be updated frequently,
either now or in the future, but I guess we don't expect that.Maybe not pg_database or pg_tablespace and such, but I'm not so sure
about pg_shdepend. (Do we record pg_shdepend entries for temp tables?)
Normal catalog access does not use HOT and never has.
If catalogs need VACUUMing then autovacuum takes care of it.
If we're saying that isn't enough and we actually depend on the
occasional user inspecting the catalog then we are already hosed.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Excerpts from Simon Riggs's message of mié nov 23 13:14:04 -0300 2011:
On Wed, Nov 23, 2011 at 3:20 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:Excerpts from Robert Haas's message of mié nov 23 12:15:55 -0300 2011:
And it effects shared catalogs only, which are all low traffic anyway.
I think "low traffic" is the key point. I understand that you're not
changing the VACUUM behavior, but you are making heap_page_prune_opt()
not do anything when a shared catalog is involved. That would be
unacceptable if we expected shared catalogs to be updated frequently,
either now or in the future, but I guess we don't expect that.Maybe not pg_database or pg_tablespace and such, but I'm not so sure
about pg_shdepend. (Do we record pg_shdepend entries for temp tables?)Normal catalog access does not use HOT and never has.
Oh.
If we're saying that isn't enough and we actually depend on the
occasional user inspecting the catalog then we are already hosed.
Probably not. I have heard of cases of pg_shdepend getting bloated
though, so it'd be nice if it happened.
--
Álvaro Herrera <alvherre@commandprompt.com>
The PostgreSQL Company - Command Prompt, Inc.
PostgreSQL Replication, Consulting, Custom Development, 24x7 support
Simon Riggs <simon@2ndQuadrant.com> writes:
On Wed, Nov 23, 2011 at 3:20 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:Maybe not pg_database or pg_tablespace and such, but I'm not so sure
about pg_shdepend. (Do we record pg_shdepend entries for temp tables?)
Normal catalog access does not use HOT and never has.
You are mistaken.
regards, tom lane
On Wed, Nov 23, 2011 at 9:44 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
On Wed, Nov 23, 2011 at 3:20 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:Excerpts from Robert Haas's message of mié nov 23 12:15:55 -0300 2011:
And it effects shared catalogs only, which are all low traffic anyway.
I think "low traffic" is the key point. I understand that you're not
changing the VACUUM behavior, but you are making heap_page_prune_opt()
not do anything when a shared catalog is involved. That would be
unacceptable if we expected shared catalogs to be updated frequently,
either now or in the future, but I guess we don't expect that.Maybe not pg_database or pg_tablespace and such, but I'm not so sure
about pg_shdepend. (Do we record pg_shdepend entries for temp tables?)Normal catalog access does not use HOT and never has.
I don't understand that. We started with the simplified assumption
that HOT can skip catalog tables, but later that was one of the
pre-conditions Tom spelled out to accept HOT patch because his view
was if this does not work for system tables, it probably does not work
at all.
Thanks,
Pavan
--
Pavan Deolasee
EnterpriseDB http://www.enterprisedb.com
On Wed, Nov 23, 2011 at 5:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Simon Riggs <simon@2ndQuadrant.com> writes:
On Wed, Nov 23, 2011 at 3:20 PM, Alvaro Herrera
<alvherre@commandprompt.com> wrote:Maybe not pg_database or pg_tablespace and such, but I'm not so sure
about pg_shdepend. (Do we record pg_shdepend entries for temp tables?)Normal catalog access does not use HOT and never has.
You are mistaken.
Normal catalog access against shared catalogs via heap_scan does not
use HOT cleanup, because it uses SnapshotNow.
Page cleanup when reading a page only happens when scan->rs_pageatatime is set.
scan->rs_pageatatime = IsMVCCSnapshot(snapshot);
Index access does use HOT cleanup, which is probably "normal".
However, since we're talking about these tables only
postgres=# select relname, pg_relation_size(oid) from pg_class where
relisshared and relkind = 'r';
relname | pg_relation_size
--------------------+------------------
pg_authid | 8192
pg_database | 8192
pg_tablespace | 8192
pg_pltemplate | 8192
pg_auth_members | 0
pg_shdepend | 8192
pg_shdescription | 8192
pg_db_role_setting | 0
(8 rows)
then I think it's fair to say that they are seldom updated/deleted and
so the effect of HOT cleanup is not important for those tables.
The real question is do we favour HOT cleanup on those small 8 tables,
or do we favour HOT cleanup of every other table? There are clearly
pros and cons but the balance must surely be in favour of better
cleaning of user tables since they are accessed millions of times more
frequently than shared catalog tables.
If we are concerned about those 8 tables then we can always set
autovacuum more intensively.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Simon Riggs <simon@2ndQuadrant.com> writes:
On Wed, Nov 23, 2011 at 5:01 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Simon Riggs <simon@2ndQuadrant.com> writes:
Normal catalog access does not use HOT and never has.
You are mistaken.
Normal catalog access against shared catalogs via heap_scan does not
use HOT cleanup, because it uses SnapshotNow.
Not sure what you are basing these statements on. Normal catalog access
typically goes through indexam.c, which AFAICS will call
heap_page_prune_opt on every heap page it visits, quite independently
of what snapshot is used. There are no cases I know of where the system
prefers heapscans on catalogs, except possibly pg_am which is known to
be small.
However, since we're talking about these tables only
...
then I think it's fair to say that they are seldom updated/deleted and
so the effect of HOT cleanup is not important for those tables.
I agree with Alvaro that pg_shdepend is probably a bit too volatile
to make such an assumption safe.
The real question is do we favour HOT cleanup on those small 8 tables,
or do we favour HOT cleanup of every other table?
No, the real question is why not think a little harder and see if we can
come up with a solution that doesn't involve making some cases worse to
make others better.
regards, tom lane
Simon Riggs <simon@2ndQuadrant.com> writes:
On Tue, Nov 22, 2011 at 11:40 PM, Simon Riggs <simon@2ndquadrant.com> wrote:
I think this is unsafe for shared catalogs.
I think so too. Thats why it uses IsMVCCSnapshot() to confirm when it
is safe to do so.
Ah, you mean access to shared catalogs using MVCC snapshots.
[ having now read the patch a bit more carefully ]
I think the fundamental problem with this is that it's conflating "what
to do in shared catalogs" with "what to do when an MVCC snapshot is
being used". HOT cleanup activity really ought not have anything at all
to do with what snapshot is being used to scan the page.
I'm also extremely uncomfortable with the fact that your proposed coding
changes not only the RecentGlobalXmin output of GetSnapshotData, but the
actual snapshot output --- you have not even made an argument why that
is safe, and I doubt that it is.
What I think might make more sense is to keep two variables,
RecentGlobalXmin with its current meaning and RecentDatabaseWideXmin
which considers only xmins of transactions in the current database.
Then HOT cleanup could select the appropriate cutoff depending on
whether it's working on a shared or non-shared relation.
regards, tom lane
On Wed, Nov 23, 2011 at 6:15 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
The real question is do we favour HOT cleanup on those small 8 tables,
or do we favour HOT cleanup of every other table?No, the real question is why not think a little harder and see if we can
come up with a solution that doesn't involve making some cases worse to
make others better.
Slightly modified patch attached.
When we access a shared relation and the page is prunable we re-derive
the cutoff value using GetOldestXmin.
That code path is rarely taken. In particular, pg_shdepend is only
accessed during object creation/alter/drop, so this isn't a path we
can't spare a small amount little extra on, just like the trade-off
we've taken to make locking faster for DML while making DDL a little
slower.
If this is still unacceptable, then I'll have to look at reducing
impact on pg_shdepend from temp tables - which is still a plan.
--
Simon Riggs http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
fix_getsnapshotdata.v3.patchapplication/octet-stream; name=fix_getsnapshotdata.v3.patchDownload+27-2
On Wed, Nov 23, 2011 at 1:30 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
What I think might make more sense is to keep two variables,
RecentGlobalXmin with its current meaning and RecentDatabaseWideXmin
which considers only xmins of transactions in the current database.
Then HOT cleanup could select the appropriate cutoff depending on
whether it's working on a shared or non-shared relation.
Unfortunately, that would have the effect of lengthening the time for
which ProcArrayLock is held, and as benchmark results from Pavan's
patch in that area show, that makes a very big difference to total
throughput on write-heavy workloads. On a related note, Simon's
proposed change here would also complicate things for that patch,
because databaseId would have to become part of PGXACT rather than
PGPROC, and that would make the PGXACT act array larger and thus
slower to scan. I have deep respect for the perils of not doing HOT
cleanup quickly enough, but ProcArrayLock contention is nothing to
sneeze at either.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
