Hot Standby and VACUUM FULL

Simon Riggs <simon@2ndQuadrant.com> writes:

The last item on my list before close is making VACUUM FULL and Hot
Standby play nicely together.

The options to do this were and still are:

(1) Add WAL messages for non-transactional relcache invalidations
(2) Allow system relations to be cluster-ed/vacuum full-ed.

(1) was how we did it originally and I believe it worked without
problem. We saw the opportunity to do (2) and it has been worth
exploring.

Refresh my memory on why (1) lets us avoid fixing (2)? It sounds
like a kluge at best ...

regards, tom lane

On Sat, 2010-01-30 at 15:17 -0500, Tom Lane wrote:

Simon Riggs <simon@2ndQuadrant.com> writes:

The last item on my list before close is making VACUUM FULL and Hot
Standby play nicely together.

The options to do this were and still are:

(1) Add WAL messages for non-transactional relcache invalidations
(2) Allow system relations to be cluster-ed/vacuum full-ed.

(1) was how we did it originally and I believe it worked without
problem. We saw the opportunity to do (2) and it has been worth
exploring.

Refresh my memory on why (1) lets us avoid fixing (2)?

(1) allows us to retain VACUUM FULL INPLACE for system relations, thus
avoiding the need to do (2). Non-transactional invalidations need to be
replayed in recovery for the same reason they exist on the primary.

It sounds like a kluge at best ...

(2) isn't a necessary change right now. It is the best design going
forwards, but its burst radius stretches far beyond Hot Standby. There
is enough code in HS for us to support, so adding to it makes little
sense for me, in this release, since there is no functional benefit in
doing so.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs wrote:

On Sat, 2010-01-30 at 15:17 -0500, Tom Lane wrote:

Simon Riggs <simon@2ndQuadrant.com> writes:

The last item on my list before close is making VACUUM FULL and Hot
Standby play nicely together.
The options to do this were and still are:
(1) Add WAL messages for non-transactional relcache invalidations
(2) Allow system relations to be cluster-ed/vacuum full-ed.
(1) was how we did it originally and I believe it worked without
problem. We saw the opportunity to do (2) and it has been worth
exploring.

Refresh my memory on why (1) lets us avoid fixing (2)?

(1) allows us to retain VACUUM FULL INPLACE for system relations, thus
avoiding the need to do (2). Non-transactional invalidations need to be
replayed in recovery for the same reason they exist on the primary.

It sounds like a kluge at best ...

(2) isn't a necessary change right now. It is the best design going
forwards, but its burst radius stretches far beyond Hot Standby. There
is enough code in HS for us to support, so adding to it makes little
sense for me, in this release, since there is no functional benefit in
doing so.

Well, it'll avoid having to support the kludges in HS required for
VACUUM FULL INPLACE.

I'm in favor of (2), unless some unforeseen hard problems come up with
implementing the ideas on that discussed earlier. Yeah, that's pretty
vague, but basically I think it's worth spending some more time doing
(2), than doing (1). For one, if the plan is to do (2) in next release
anyway, we might as well do it now and avoid having to support the
HS+VACUUM FULL INPLACE combination in only 9.0 stable branch for years
to come.

I believe we had a pretty well-thought out plan on how to support system
catalogs with the new VACUUM FULL.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Sun, 2010-01-31 at 21:00 +0200, Heikki Linnakangas wrote:

Simon Riggs wrote:

On Sat, 2010-01-30 at 15:17 -0500, Tom Lane wrote:

Simon Riggs <simon@2ndQuadrant.com> writes:

The last item on my list before close is making VACUUM FULL and Hot
Standby play nicely together.
The options to do this were and still are:
(1) Add WAL messages for non-transactional relcache invalidations
(2) Allow system relations to be cluster-ed/vacuum full-ed.
(1) was how we did it originally and I believe it worked without
problem. We saw the opportunity to do (2) and it has been worth
exploring.

Refresh my memory on why (1) lets us avoid fixing (2)?

(1) allows us to retain VACUUM FULL INPLACE for system relations, thus
avoiding the need to do (2). Non-transactional invalidations need to be
replayed in recovery for the same reason they exist on the primary.

It sounds like a kluge at best ...

(2) isn't a necessary change right now. It is the best design going
forwards, but its burst radius stretches far beyond Hot Standby. There
is enough code in HS for us to support, so adding to it makes little
sense for me, in this release, since there is no functional benefit in
doing so.

Well, it'll avoid having to support the kludges in HS required for
VACUUM FULL INPLACE.

I'm in favor of (2), unless some unforeseen hard problems come up with
implementing the ideas on that discussed earlier. Yeah, that's pretty
vague, but basically I think it's worth spending some more time doing
(2), than doing (1). For one, if the plan is to do (2) in next release
anyway, we might as well do it now and avoid having to support the
HS+VACUUM FULL INPLACE combination in only 9.0 stable branch for years
to come.

That's a good argument, but with respect, it isn't you who is writing
the code, nor will it be you that supports it, AIUI. Right now, HS is
isolated in many ways. If we introduce this change it will effect
everybody and that means I'll be investigating all manner of bug reports
that have zip to do with HS for a long time to come.

What I would say is that for 9.0 we can easily remove the INPLACE option
as an explicit request.

I believe we had a pretty well-thought out plan on how to support system
catalogs with the new VACUUM FULL.

I think calling it a "well thought out plan" is, err, overstating
things. We had what looked like a viable sketch of how to proceed and I
agreed to investigate that. Having done so, I'm saying I don't like what
I see going further and wish to backtrack to my known safe solution.

Overall, I don't see any benefit in pursuing that course any further. I
just see risk, on balance with (2), whereas (1) seems easier/faster,
less risk and more isolated.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon@2ndQuadrant.com> writes:

On Sat, 2010-01-30 at 15:17 -0500, Tom Lane wrote:

Simon Riggs <simon@2ndQuadrant.com> writes:

The options to do this were and still are:
(1) Add WAL messages for non-transactional relcache invalidations
(2) Allow system relations to be cluster-ed/vacuum full-ed.

Refresh my memory on why (1) lets us avoid fixing (2)?

(1) allows us to retain VACUUM FULL INPLACE for system relations, thus
avoiding the need to do (2). Non-transactional invalidations need to be
replayed in recovery for the same reason they exist on the primary.

Well, I would expect that invalidation events need to be transmitted to
hot-standby slaves no matter what --- backends running queries on an HS
slave need to hear about inval events just as much as backends on the
master do. So my take on it is that all inval events will have to have
associated WAL records when in HS mode, independently of what we choose
to do about VACUUM.

Anyway, it's still not apparent to me exactly what the connection is
between VACUUM FULL and Hot Standby. I remember that we said HS didn't
work with VACUUM FULL (INPLACE) but I don't recall why that is, and the
links on the open-items pages are not leading me to any useful
discussion.

regards, tom lane

Simon Riggs <simon@2ndQuadrant.com> writes:

Having said that I now realise a few things I didn't before:

* Approach (2) effects the core of Postgres, even if you don't use Hot
Standby.
* I've had to remove 7 sanity checks to get the first few VACUUMs
working. ISTM that removing various basic checks in the code is not a
good thing.
* There are are more special cases than I realised at first: temp,
shared, with-toast, nailed, shared-and-nailed, pg_class, normal system.

Quite honestly, these statements and the attached patch (which doesn't
even begin to touch the central issue, but does indeed break a lot of
things) demonstrate that *you* are not the guy to implement what was
being discussed. It needs to be done by someone who understands the
core caching code, which apparently you haven't studied in any detail.

I have a feeling that I should go do this...

regards, tom lane

On Sun, 2010-01-31 at 14:35 -0500, Tom Lane wrote:

Simon Riggs <simon@2ndQuadrant.com> writes:

On Sat, 2010-01-30 at 15:17 -0500, Tom Lane wrote:

Simon Riggs <simon@2ndQuadrant.com> writes:

The options to do this were and still are:
(1) Add WAL messages for non-transactional relcache invalidations
(2) Allow system relations to be cluster-ed/vacuum full-ed.

Refresh my memory on why (1) lets us avoid fixing (2)?

(1) allows us to retain VACUUM FULL INPLACE for system relations, thus
avoiding the need to do (2). Non-transactional invalidations need to be
replayed in recovery for the same reason they exist on the primary.

Well, I would expect that invalidation events need to be transmitted to
hot-standby slaves no matter what --- backends running queries on an HS
slave need to hear about inval events just as much as backends on the
master do. So my take on it is that all inval events will have to have
associated WAL records when in HS mode, independently of what we choose
to do about VACUUM.

All transactional invalidations are already handled by HS. It was the
non-transactional invalidations in VACUUM FULL INPLACE that still
require additional handling.

Anyway, it's still not apparent to me exactly what the connection is
between VACUUM FULL and Hot Standby. I remember that we said HS didn't
work with VACUUM FULL (INPLACE) but I don't recall why that is, and the
links on the open-items pages are not leading me to any useful
discussion.

Very little really; not enough to force the sort of changes that I am
now seeing will be required in the way catalogs and caches operate.
There was some difficulty around the fact that VFI issues two commits
for the same transaction, but that is now correctly handled in the code
after discussion.

I'm not known as a risk-averse person but (2) is a step too far for me.

--
Simon Riggs www.2ndQuadrant.com

On Sun, 2010-01-31 at 14:44 -0500, Tom Lane wrote:

Simon Riggs <simon@2ndQuadrant.com> writes:

Having said that I now realise a few things I didn't before:

* Approach (2) effects the core of Postgres, even if you don't use Hot
Standby.
* I've had to remove 7 sanity checks to get the first few VACUUMs
working. ISTM that removing various basic checks in the code is not a
good thing.
* There are are more special cases than I realised at first: temp,
shared, with-toast, nailed, shared-and-nailed, pg_class, normal system.

Quite honestly, these statements and the attached patch (which doesn't
even begin to touch the central issue, but does indeed break a lot of
things) demonstrate that *you* are not the guy to implement what was
being discussed. It needs to be done by someone who understands the
core caching code, which apparently you haven't studied in any detail.

I didn't claim the attached patch began to touch the issues. I was very
clear that it covered only the very simplest use case, that was the
point. You may not wish to acknowledge it, but I *have* studied the core
caching code in detail for many months and that is the basis for my
comments. The way I have written the exclusions in vacuum.c shows that I
have identified each of the sub-cases we are required to handle.

There is nothing wrong with your idea of using a mapping file. That is
relatively easy part of the problem.

I have a feeling that I should go do this...

If you wish, but I still think it is an unnecessary change for this
release, whoever does it. We both know that once you start you won't
stop, but that doesn't make it worthwhile or less risky.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon@2ndQuadrant.com> writes:

On Sun, 2010-01-31 at 14:35 -0500, Tom Lane wrote:

Anyway, it's still not apparent to me exactly what the connection is
between VACUUM FULL and Hot Standby. I remember that we said HS didn't
work with VACUUM FULL (INPLACE) but I don't recall why that is, and the

[ sorry, I meant not-INPLACE ]

links on the open-items pages are not leading me to any useful
discussion.

Very little really; not enough to force the sort of changes that I am
now seeing will be required in the way catalogs and caches operate.
There was some difficulty around the fact that VFI issues two commits
for the same transaction, but that is now correctly handled in the code
after discussion.

If the only benefit of getting rid of VACUUM FULL were simplifying
Hot Standby, I'd agree with you. But there are numerous other benefits.
The double-commit hack you mention is something we need to get rid of
for general system stability (because of the risk of PANIC if the vacuum
fails after the first commit). Getting rid of REINDEX-in-place on
shared catalog indexes is another thing that's really safety critical.
Removing V-F related hacks in other places would just be a bonus.

It's something we need to do, so if Hot Standby is forcing our hands,
then let's just do it.

regards, tom lane

On Sun, 2010-01-31 at 15:14 -0500, Tom Lane wrote:

Simon Riggs <simon@2ndQuadrant.com> writes:

On Sun, 2010-01-31 at 14:35 -0500, Tom Lane wrote:

Anyway, it's still not apparent to me exactly what the connection is
between VACUUM FULL and Hot Standby. I remember that we said HS didn't
work with VACUUM FULL (INPLACE) but I don't recall why that is, and the

[ sorry, I meant not-INPLACE ]

links on the open-items pages are not leading me to any useful
discussion.

Very little really; not enough to force the sort of changes that I am
now seeing will be required in the way catalogs and caches operate.
There was some difficulty around the fact that VFI issues two commits
for the same transaction, but that is now correctly handled in the code
after discussion.

If the only benefit of getting rid of VACUUM FULL were simplifying
Hot Standby, I'd agree with you. But there are numerous other benefits.
The double-commit hack you mention is something we need to get rid of
for general system stability (because of the risk of PANIC if the vacuum
fails after the first commit). Getting rid of REINDEX-in-place on
shared catalog indexes is another thing that's really safety critical.
Removing V-F related hacks in other places would just be a bonus.

It's something we need to do, so if Hot Standby is forcing our hands,
then let's just do it.

That's the point: Hot Standby is *not* forcing our hand to do this.

Doing this will not simplify Hot Standby in any significant way. The
code to support VFI with Hot Standby is, after technical review, much,
much simpler than the code to remove VFI.

I'll do a little work towards step (1) just so we can take a more
informed view once you've had a better look at just what (2) involves. I
had already written the code for the Sept release of HS.

--
Simon Riggs www.2ndQuadrant.com

On Sun, 2010-01-31 at 15:14 -0500, Tom Lane wrote:

If the only benefit of getting rid of VACUUM FULL were simplifying
Hot Standby, I'd agree with you. But there are numerous other benefits.
The double-commit hack you mention is something we need to get rid of
for general system stability (because of the risk of PANIC if the vacuum
fails after the first commit). Getting rid of REINDEX-in-place on
shared catalog indexes is another thing that's really safety critical.
Removing V-F related hacks in other places would just be a bonus.

I should've agreed with this in my last post, cos I do. I want very,
very much to get rid of VACUUM FULL just because it's such a sump of
ugly, complex code. But there is a limit to how and when performs what I
now see is a more major surgical operation.

--
Simon Riggs www.2ndQuadrant.com

Simon Riggs <simon@2ndQuadrant.com> writes:

I'll do a little work towards step (1) just so we can take a more
informed view once you've had a better look at just what (2) involves.

I spent a couple of hours reading code and believe that I've identified
all the pain points for allowing relocation of system catalogs (ie,
assigning them new relfilenodes during cluster-style VACUUM FULL,
REINDEX, etc). It's definitely not a trivial project but it's not out
of reach either --- I estimate I could get it done in a couple or three
days of full-time effort. Given the potential benefits I think it's
worth doing. Rough notes are attached --- comments?

regards, tom lane

Change pg_class.relfilenode to be 0 for all rels for which a map file should
be used instead. Define RelationGetFilenode() to look to the physaddr info
instead, and make all internal refs go to that instead of reading
rd_rel->relfilenode directly. Define pg_relation_filenode(regclass) and fix
external refs (oid2name, pg_dump) to use that instead.

In zeroth cut, just have relcache.c substitute the OID in place of reading
a map file. Possibly it should always do that during bootstrap.

How should bootstrap decide which rels to insert zero for, anyway?
Shared definitely, pg_class, ... maybe that's enough? Or do we need
it for all nailed local catalogs?

local state contains
* shared map list
* this database's map list
* list of local overrides to each on-disk map list

At commit: if modified, write out new version; do this as late as possible
before xact commit
At abort: just discard local-override list

"Write" must include generating a WAL entry as well as sending a shared-inval
signal
* write temp file, fsync it
* emit WAL record containing copy of new file contents, fsync it
* atomically rename temp file into place
* send sinval

During relation open, check for sinval before relying on current cached value
of map contents

Hm, what about concurrent updates of map? Probably instantiate a new
LWLock or maybe better a HW lock. So write involves taking lock, check
for updates, finally write --- which means that local modifications
have to be stored in a way that allows re-reading somebody else's mods.
Hence above data structure with separate storage of local modifications.
We assume rel-level locking protects us from concurrent update attempts
on the same map entry, but we don't want to forbid concurrent relocations
of different catalogs.

LWLock would work if we do an unconditional read of the file contents after
getting lock rather than relying on sinval signaling, which seems a small
price considering map updates should be infrequent. Without that, writers
have to hold the lock till commit which rules out using LWLock.

Hm ... do we want an LWLock per map file, or is one lock to rule them all
sufficient? LWLock per database seems problematic. With an HW lock there
wouldn't be a problem with that. HW lock would allow concurrent updates of
the map files of different DBs, but is that worth the extra cycles?

In a case where a transaction relocates pg_class (or another mapped catalog)
and then makes updates in that catalog, all is well in the sense that the
updates will be preserved iff the xact commits. HOWEVER we would have
problems during WAL replay? No, because the WAL entries will command updates
to the catalog's new relfilenode, which will be interesting to anyone else if
and only if the xact gets to commit. We would need to be careful about the
case of relocating pg_class itself though --- make sure local relcache
references the new pg_class before any such updates happen. Probably a CCI
is sufficient.

Another issue for clustering a catalog is that sinval catcache signalling
could get confused, since that depends on catalog entry TIDs which would
change --- we'd likely need to have relocation send an sinval signal saying
"flush *everything* from that catalog". (relcache inval on the catalog itself
doesn't do that.) This action could/should be transactional so no
additional code is needed to propagate the notice to HS standbys.

Once the updated map file is moved into place, the relocation is effectively
committed even if we subsequently abort the transaction. We can make that
window pretty narrow but not remove it completely. Risk factors:
* abort would try to remove relation files created by xact, in particular
the new version of the catalog. Ooops. Can fix this by telling smgr to
forget about removing those files before installing the new map file ---
better to leak some disk space than destroy catalogs.
* on abort, we'd not send sinval notices, leaving other backends at risk
of using stale info (maybe our own too). We could fix this by sending
the sinval notice BEFORE renaming into place (if we send it and then fail
to rename, no real harm done, just useless cache flushes). This requires
keeping a nontransactional-inval path in inval.c, but at least it's much
more localized than before. No problem for HS since we have the WAL record
for map update to drive issuing sinvals on the slave side. Note this
means that readers need to take the mapfile lock in shared mode, since they
could conceivably get the sinval notice before we complete the rename.

For our own backend we need cache flushes at CCI and again at xact
commit/abort. This could be handled by regular transactional inval
path but we end up with a lot of redundant flushing. Maybe not worth
worrying about though.

Disallow catalog relocation inside subtransactions, to avoid having
to handle subxact abort effects on the local-map-changes state.
This could be implemented if desired, but doesn't seem worth it
at least in first pass.

FYI, getting rid of VACUUM FULL in a .0 release does make more sense
than doing it in a .1 release.

---------------------------------------------------------------------------

Tom Lane wrote:

Simon Riggs <simon@2ndQuadrant.com> writes:

I'll do a little work towards step (1) just so we can take a more
informed view once you've had a better look at just what (2) involves.

I spent a couple of hours reading code and believe that I've identified
all the pain points for allowing relocation of system catalogs (ie,
assigning them new relfilenodes during cluster-style VACUUM FULL,
REINDEX, etc). It's definitely not a trivial project but it's not out
of reach either --- I estimate I could get it done in a couple or three
days of full-time effort. Given the potential benefits I think it's
worth doing. Rough notes are attached --- comments?

regards, tom lane

Change pg_class.relfilenode to be 0 for all rels for which a map file should
be used instead. Define RelationGetFilenode() to look to the physaddr info
instead, and make all internal refs go to that instead of reading
rd_rel->relfilenode directly. Define pg_relation_filenode(regclass) and fix
external refs (oid2name, pg_dump) to use that instead.

In zeroth cut, just have relcache.c substitute the OID in place of reading
a map file. Possibly it should always do that during bootstrap.

How should bootstrap decide which rels to insert zero for, anyway?
Shared definitely, pg_class, ... maybe that's enough? Or do we need
it for all nailed local catalogs?

local state contains
* shared map list
* this database's map list
* list of local overrides to each on-disk map list

At commit: if modified, write out new version; do this as late as possible
before xact commit
At abort: just discard local-override list

"Write" must include generating a WAL entry as well as sending a shared-inval
signal
* write temp file, fsync it
* emit WAL record containing copy of new file contents, fsync it
* atomically rename temp file into place
* send sinval

During relation open, check for sinval before relying on current cached value
of map contents

Hm, what about concurrent updates of map? Probably instantiate a new
LWLock or maybe better a HW lock. So write involves taking lock, check
for updates, finally write --- which means that local modifications
have to be stored in a way that allows re-reading somebody else's mods.
Hence above data structure with separate storage of local modifications.
We assume rel-level locking protects us from concurrent update attempts
on the same map entry, but we don't want to forbid concurrent relocations
of different catalogs.

LWLock would work if we do an unconditional read of the file contents after
getting lock rather than relying on sinval signaling, which seems a small
price considering map updates should be infrequent. Without that, writers
have to hold the lock till commit which rules out using LWLock.

Hm ... do we want an LWLock per map file, or is one lock to rule them all
sufficient? LWLock per database seems problematic. With an HW lock there
wouldn't be a problem with that. HW lock would allow concurrent updates of
the map files of different DBs, but is that worth the extra cycles?

In a case where a transaction relocates pg_class (or another mapped catalog)
and then makes updates in that catalog, all is well in the sense that the
updates will be preserved iff the xact commits. HOWEVER we would have
problems during WAL replay? No, because the WAL entries will command updates
to the catalog's new relfilenode, which will be interesting to anyone else if
and only if the xact gets to commit. We would need to be careful about the
case of relocating pg_class itself though --- make sure local relcache
references the new pg_class before any such updates happen. Probably a CCI
is sufficient.

Another issue for clustering a catalog is that sinval catcache signalling
could get confused, since that depends on catalog entry TIDs which would
change --- we'd likely need to have relocation send an sinval signal saying
"flush *everything* from that catalog". (relcache inval on the catalog itself
doesn't do that.) This action could/should be transactional so no
additional code is needed to propagate the notice to HS standbys.

Once the updated map file is moved into place, the relocation is effectively
committed even if we subsequently abort the transaction. We can make that
window pretty narrow but not remove it completely. Risk factors:
* abort would try to remove relation files created by xact, in particular
the new version of the catalog. Ooops. Can fix this by telling smgr to
forget about removing those files before installing the new map file ---
better to leak some disk space than destroy catalogs.
* on abort, we'd not send sinval notices, leaving other backends at risk
of using stale info (maybe our own too). We could fix this by sending
the sinval notice BEFORE renaming into place (if we send it and then fail
to rename, no real harm done, just useless cache flushes). This requires
keeping a nontransactional-inval path in inval.c, but at least it's much
more localized than before. No problem for HS since we have the WAL record
for map update to drive issuing sinvals on the slave side. Note this
means that readers need to take the mapfile lock in shared mode, since they
could conceivably get the sinval notice before we complete the rename.

For our own backend we need cache flushes at CCI and again at xact
commit/abort. This could be handled by regular transactional inval
path but we end up with a lot of redundant flushing. Maybe not worth
worrying about though.

Disallow catalog relocation inside subtransactions, to avoid having
to handle subxact abort effects on the local-map-changes state.
This could be implemented if desired, but doesn't seem worth it
at least in first pass.

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

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ If your life is a hard drive, Christ can be your backup. +

Tom Lane wrote:

Hm ... do we want an LWLock per map file, or is one lock to rule them all
sufficient? LWLock per database seems problematic. With an HW lock there
wouldn't be a problem with that. HW lock would allow concurrent updates of
the map files of different DBs, but is that worth the extra cycles?

A single LWLock should be enough.

Once the updated map file is moved into place, the relocation is effectively
committed even if we subsequently abort the transaction. We can make that
window pretty narrow but not remove it completely.

We could include the instructions to update the map file in the commit
record, instead of introducing a new record type, and update the map
file only *after* writing the commit record. The map file doesn't grow,
so we can be pretty confident that updating it doesn't fail (failure
would lead to PANIC).

I'm assuming the map file is fixed size, with a fixed location for each
relation, so that we can just overwrite the old file without the
create+rename dance, and not worry about torn-pages.

--
Heikki Linnakangas
EnterpriseDB http://www.enterprisedb.com

On Sun, 2010-01-31 at 22:49 -0500, Tom Lane wrote:

Simon Riggs <simon@2ndQuadrant.com> writes:

I'll do a little work towards step (1) just so we can take a more
informed view once you've had a better look at just what (2) involves.

I spent a couple of hours reading code and believe that I've identified
all the pain points for allowing relocation of system catalogs (ie,
assigning them new relfilenodes during cluster-style VACUUM FULL,
REINDEX, etc). It's definitely not a trivial project but it's not out
of reach either --- I estimate I could get it done in a couple or three
days of full-time effort. Given the potential benefits I think it's
worth doing. Rough notes are attached --- comments?

Comments inline.

Change pg_class.relfilenode to be 0 for all rels for which a map file should
be used instead. Define RelationGetFilenode() to look to the physaddr info
instead, and make all internal refs go to that instead of reading
rd_rel->relfilenode directly.

Yes

Define pg_relation_filenode(regclass) and fix
external refs (oid2name, pg_dump) to use that instead.

Not sure why?

In zeroth cut, just have relcache.c substitute the OID in place of reading
a map file. Possibly it should always do that during bootstrap.

Yes

How should bootstrap decide which rels to insert zero for, anyway?
Shared definitely, pg_class, ... maybe that's enough? Or do we need
it for all nailed local catalogs?

Only for nailed || shared. My submitted patch covers the "normal"
relations.

Put 0 for rel at boot, write files at boot also.

local state contains
* shared map list
* this database's map list
* list of local overrides to each on-disk map list

At commit: if modified, write out new version; do this as late as possible
before xact commit
At abort: just discard local-override list

Yep

"Write" must include generating a WAL entry as well as sending a shared-inval
signal
* write temp file, fsync it
* emit WAL record containing copy of new file contents, fsync it
* atomically rename temp file into place
* send sinval

Yes

During relation open, check for sinval before relying on current cached value
of map contents

Yes

Hm, what about concurrent updates of map?

Why not have one file per relation that has a map file? No concurrency
issues then at all then.

Another issue for clustering a catalog is that sinval catcache signalling
could get confused, since that depends on catalog entry TIDs which would
change --- we'd likely need to have relocation send an sinval signal saying
"flush *everything* from that catalog". (relcache inval on the catalog itself
doesn't do that.)

Yes

This action could/should be transactional so no
additional code is needed to propagate the notice to HS standbys.

Agreed

Once the updated map file is moved into place, the relocation is effectively
committed even if we subsequently abort the transaction. We can make that
window pretty narrow but not remove it completely. Risk factors:
* abort would try to remove relation files created by xact, in particular
the new version of the catalog. Ooops. Can fix this by telling smgr to
forget about removing those files before installing the new map file ---
better to leak some disk space than destroy catalogs.
* on abort, we'd not send sinval notices, leaving other backends at risk
of using stale info (maybe our own too). We could fix this by sending
the sinval notice BEFORE renaming into place (if we send it and then fail
to rename, no real harm done, just useless cache flushes). This requires
keeping a nontransactional-inval path in inval.c, but at least it's much
more localized than before. No problem for HS since we have the WAL record
for map update to drive issuing sinvals on the slave side. Note this
means that readers need to take the mapfile lock in shared mode, since they
could conceivably get the sinval notice before we complete the rename.

For our own backend we need cache flushes at CCI and again at xact
commit/abort. This could be handled by regular transactional inval
path but we end up with a lot of redundant flushing. Maybe not worth
worrying about though.

!

Disallow catalog relocation inside subtransactions, to avoid having
to handle subxact abort effects on the local-map-changes state.
This could be implemented if desired, but doesn't seem worth it
at least in first pass.

Agreed, not needed for emergency maintenance actions like this.

There are issues associated with performing actions on critical tables,
since in some cases I got "relation xxxx is already locked". ISTM we'd
need some special handling of locking in those cases.

Toast tables are also an area of problem because there are dependencies
between tables that need to be mangled.

Please bear in mind that I looked at the code also and came to a similar
original assessment. It was only when I started working with it that I
came to different conclusions.

--
Simon Riggs www.2ndQuadrant.com

On Mon, Feb 1, 2010 at 8:54 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

Disallow catalog relocation inside subtransactions, to avoid having
to handle subxact abort effects on the local-map-changes state.
This could be implemented if desired, but doesn't seem worth it
at least in first pass.

Agreed, not needed for emergency maintenance actions like this.

Note that this would mean it will never work if you have psql's
ROLLBACK_ON_ERROR set.

--
greg

Heikki Linnakangas <heikki.linnakangas@enterprisedb.com> writes:

Tom Lane wrote:

Once the updated map file is moved into place, the relocation is effectively
committed even if we subsequently abort the transaction. We can make that
window pretty narrow but not remove it completely.

We could include the instructions to update the map file in the commit
record, instead of introducing a new record type, and update the map
file only *after* writing the commit record. The map file doesn't grow,
so we can be pretty confident that updating it doesn't fail (failure
would lead to PANIC).

I'm assuming the map file is fixed size, with a fixed location for each
relation, so that we can just overwrite the old file without the
create+rename dance, and not worry about torn-pages.

That seems too fragile to me, as I don't find it a stretch at all to
think that writing the map file might fail --- just think Windows
antivirus code :-(. Now, once we have written the WAL record for
the mapfile change, we can't really afford a failure in my approach
either. But I think a rename() after successfully creating/writing/
fsync'ing a temp file is a whole lot safer than writing from a standing
start.

The other problem with what you sketch is that it'd require holding the
mapfile write lock across commit, because we still have to have strict
serialization of updates.

[ thinks for awhile ... ] OTOH, overwrite-in-place is what we've always
used for pg_control updates, and I don't recall ever seeing a report of
a problem that could be traced to that. Maybe we should forget the
rename() trick and overwrite the map file in place. I still think it
needs to be a separate WAL record though. I'm thinking

* obtain lock
* open file for read/write
* read current contents
* construct modified contents
* write and sync WAL record
* write back file through already-opened descriptor
* fsync
* release lock

Not totally clear if this is more or less safe than the rename method;
but given the assumption that the file is less than one disk block,
it should be just as atomic as pg_control updates are.

regards, tom lane

Greg Stark <gsstark@mit.edu> writes:

On Mon, Feb 1, 2010 at 8:54 AM, Simon Riggs <simon@2ndquadrant.com> wrote:

Disallow catalog relocation inside subtransactions, to avoid having
to handle subxact abort effects on the local-map-changes state.
This could be implemented if desired, but doesn't seem worth it
at least in first pass.

Agreed, not needed for emergency maintenance actions like this.

Note that this would mean it will never work if you have psql's
ROLLBACK_ON_ERROR set.

VACUUM has always failed in such a case, so I don't see this as a
showstopper.

regards, tom lane

On Mon, 2010-02-01 at 10:06 -0500, Tom Lane wrote:

the assumption that the file is less than one disk block,
it should be just as atomic as pg_control updates are.

IIRC there were 173 relations affected by this. 4 bytes each we would
have more than 512 bytes.

ISTM you need to treat some of those system relations just as normal
relations rather than give everybody a map entry.

--
Simon Riggs www.2ndQuadrant.com