CLUSTER patch

I am working on the same thing myself. Attached is a version of the
patch that applies to current CVS. I worked with
RelationClearRelation() to clear the cache for the relations.

It now reports "Relation still open" during index_drop(), so something
still isn't right. I think the whole problem stems from the fact that
the relation cache caches by relnode, so when we change it, the cache
gets all confused. The RelationClearRelation() change handles this in a
very crude way, but it seems to be in the right direction for a
solution.

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

Alvaro Herrera wrote:

Hello:

I think I have some more knowledge on my failing to build a working
CLUSTER patch. It does not help me to fix it, however.

The way the cluster is done is still the same: first cluster the heap
and swap relfilenodes, then drop old heap; then rebuild each index, swap
relfilenodes with old index and drop new.

If I don't use the new relfilenode for anything after building it,
everything works OK. But I can't attach the old filenode to the new
heap; it has to stay around. I do can attach the new filenode to the
old heap however. But as soon as I try to do anything to it (the new,
clustered filenode) before transaction commit (building an index, say),
the local buffer manager fails an assertion (actually,
RelationNodeCacheGetRelation returns 0 for the given rnode), and the
transaction aborts.

Thus, if I comment the lines that attach the old rnode to the new heap
and the lines that create the new indexes, the cluster works, and I now
have two tables with the same rnode, one of the named
"temp_<oid-of-the-other>". Obviously dropping any of those renders the
other useless (no disk storage).

What I still don't understand is why the RelationNodeCache doesn't
return the buffer I'm looking for. I added some debugging fprintf's to
the bufmgr code and the relation _is_ entered (!?) during the cluster
transaction. Maybe somewhere the local buffer manager drops the
knowledge about the relation, or that knowledge is based on (OID +
rnode) info, which changed because the rnode changed. I do not
understand the relcaching bussiness anyway.

I attach anyway the old version of the patch, that reconstructs the
indexes without the filenode swapping bussiness. The observations done
to the earlier patch are corrected. I think that if nothing comes to
fix the problems with the newer approach and there are no other
objections, this one should be applied as it enhances the current
version anyway.

I'm leaving for winter vacation on monday and probably won't be back on
two weeks.

--
Alvaro Herrera (<alvherre[a]atentus.com>)
"La vida es para el que se aventura"

Content-Description:

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 6: Have you searched our list archives?

http://archives.postgresql.org

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Alvaro Herrera <alvherre@atentus.com> writes:

The way the cluster is done is still the same: first cluster the heap
and swap relfilenodes, then drop old heap; then rebuild each index, swap
relfilenodes with old index and drop new.

I do not see anything in this patch that touches relfilenode. Perhaps
the patch is incomplete?

But as soon as I try to do anything to it (the new,
clustered filenode) before transaction commit (building an index, say),
the local buffer manager fails an assertion (actually,
RelationNodeCacheGetRelation returns 0 for the given rnode), and the
transaction aborts.

Hmm. If you do the swap in the correct way (viz, update the relation's
pg_class entry and then CommandCounterIncrement) I'd expect the relcache
to respond correctly. This does involve re-indexing the relcache entry
under a new relfilenode value, but that's not significantly different
from the case of renaming a relation.

regards, tom lane

Bruce Momjian <pgman@candle.pha.pa.us> writes:

+ 	CommandCounterIncrement();
+ 
+ 	// bjm
+ //	RelationIdInvalidateRelationCacheByRelationId(r1);
+ //	RelationIdInvalidateRelationCacheByRelationId(r2);
+ 
+ 	RelationClearRelation(RelationIdGetRelation(r1), true);
+ //   	RelationClearRelation(RelationIdGetRelation(r2), true);
+ 
+ 	CommandCounterIncrement();

Surely the above is neither necessary nor appropriate. The relcache
should automatically rebuild its entries for these relations at
CommandCounterIncrement. In any case I do not care for exporting
internal relcache routines to make CLUSTER work ...

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

+ 	CommandCounterIncrement();
+ 
+ 	// bjm
+ //	RelationIdInvalidateRelationCacheByRelationId(r1);
+ //	RelationIdInvalidateRelationCacheByRelationId(r2);
+ 
+ 	RelationClearRelation(RelationIdGetRelation(r1), true);
+ //   	RelationClearRelation(RelationIdGetRelation(r2), true);
+ 
+ 	CommandCounterIncrement();

Surely the above is neither necessary nor appropriate. The relcache
should automatically rebuild its entries for these relations at

New patch attached. Something like this is required or
heap_drop/index_drop will fail because it can't find the relation cache
entries for the relation. Maybe the trick is to properly invalidate the
relation caches when pg_class is updated. This is particularly
significant for thisxactonly relations. I don't think we need to handle
oid changes in pg_class, but relfilenode changes are not updated,
causing problems down the road. The attached patch actually does work
with the following warnings:

test=> cluster i on test;
WARNING: trying to delete a reldesc (rd_node) that does not exist.
WARNING: trying to delete a reldesc (rd_node) that does not exist.
CLUSTER

My guess is that the proper fix is elsewhere. I looked through
relcache.c and didn't see any case where a relfilenode could be
invalidated _without_ removing the old entry first, but of course, the
old entry has a different value from the new one. My guess is that the
work is to be done there somewhere.

My cache forget calls work because it forces new cache entries to match
pg_class, and that way the other commands can succeed.

CommandCounterIncrement. In any case I do not care for exporting
internal relcache routines to make CLUSTER work ...

I need to get cluster working before I can worry about style.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Tom Lane dijo:

Alvaro Herrera <alvherre@atentus.com> writes:

The way the cluster is done is still the same: first cluster the heap
and swap relfilenodes, then drop old heap; then rebuild each index, swap
relfilenodes with old index and drop new.

I do not see anything in this patch that touches relfilenode. Perhaps
the patch is incomplete?

No, this is the old version, corrected according your comments, for
inclusion in case the new version doesn't make it.

Perhaps you missed the patch I posted some days ago that did swap
relfilenodes (there was even a function named swap_relfilenode, so it
wasn't easy to miss). It's archived in
http://archives.postgresql.org/pgsql-patches/2002-07/msg00079.php. The
code in question is this:

tempRFNode = ((Form_pg_class) GETSTRUCT(reltup[0]))->relfilenode;
((Form_pg_class) GETSTRUCT(reltup[0]))->relfilenode =
((Form_pg_class) GETSTRUCT(reltup[1]))->relfilenode;
((Form_pg_class) GETSTRUCT(reltup[1]))->relfilenode = tempRFNode;

/* Update the RelationRelationName tuples */
simple_heap_update(relRelation, &reltup[1]->t_self, reltup[1]);
simple_heap_update(relRelation, &reltup[0]->t_self, reltup[0]);

Now, if I do CommandCounterIncrement() right after this, I get "Relation
deleted while still in use". So I add

CatalogOpenIndices(Num_pg_class_indices, Name_pg_class_indices, irels);
CatalogIndexInsert(irels, Num_pg_class_indices, relRelation, reltup[1]);
CatalogIndexInsert(irels, Num_pg_class_indices, relRelation, reltup[0]);
CatalogCloseIndices(Num_pg_class_indices, irels);

and only then the CommandCounterIncrement. But the server says
TRAP: Failed Assertion("!(bufrel != ((void *)0)):", File: "localbuf.c",
Line: 242)

(I think it's line 241 in pristine source). I tried doing
CatalogIndexInsert and CommandCounterIncrement after each
simple_heap_update, but the result is the same. This assertion is
failed at transaction commit, when LocalBufferSync is called.

But as soon as I try to do anything to it (the new,
clustered filenode) before transaction commit (building an index, say),
the local buffer manager fails an assertion (actually,
RelationNodeCacheGetRelation returns 0 for the given rnode), and the
transaction aborts.

Hmm. If you do the swap in the correct way (viz, update the relation's
pg_class entry and then CommandCounterIncrement) I'd expect the relcache
to respond correctly. This does involve re-indexing the relcache entry
under a new relfilenode value, but that's not significantly different
from the case of renaming a relation.

That's what I think I'm doing, but I don't know what's the relcache
doing; other than I expect, I pressume. I tried using
RelationForgetRelation on both relations (following Bruce's idea of
invalidating the relcache entries), but I get lost in the relcache, so
I'm now in the dark.

--
Alvaro Herrera (<alvherre[a]atentus.com>)
"The ability to monopolize a planet is insignificant
next to the power of the source"

Bruce Momjian <pgman@candle.pha.pa.us> writes:

New patch attached. Something like this is required or
heap_drop/index_drop will fail because it can't find the relation cache
entries for the relation. Maybe the trick is to properly invalidate the
relation caches when pg_class is updated.

They should be updated *automatically* --- otherwise CLUSTER is hardly
the only thing that will fail.

This is particularly
significant for thisxactonly relations.

Yes. After thinking awhile I realize that the real problem is that we
are trying to swap between an existing relation (!rd_myxactonly) and
a new relation (rd_myxactonly). Buffers for one live in the main
buffer pool, for the other in the local buffer pool. There's also the
little matter of the local state inside relcache.c. While the update
to pg_class should make the right things happen to relfilenode, it
doesn't do anything to cause a change in rd_myxactonly status.

Not sure what to do about this. Will think more.

regards, tom lane

Alvaro Herrera wrote:

Tom Lane dijo:

Alvaro Herrera <alvherre@atentus.com> writes:

The way the cluster is done is still the same: first cluster the heap
and swap relfilenodes, then drop old heap; then rebuild each index, swap
relfilenodes with old index and drop new.

I do not see anything in this patch that touches relfilenode. Perhaps
the patch is incomplete?

No, this is the old version, corrected according your comments, for
inclusion in case the new version doesn't make it.

I sent a new version of your patch out that got farther, and it does
have those swap lines.

Now, if I do CommandCounterIncrement() right after this, I get "Relation
deleted while still in use". So I add

CatalogOpenIndices(Num_pg_class_indices, Name_pg_class_indices, irels);
CatalogIndexInsert(irels, Num_pg_class_indices, relRelation, reltup[1]);
CatalogIndexInsert(irels, Num_pg_class_indices, relRelation, reltup[0]);
CatalogCloseIndices(Num_pg_class_indices, irels);

Yes, that is required.

and only then the CommandCounterIncrement. But the server says
TRAP: Failed Assertion("!(bufrel != ((void *)0)):", File: "localbuf.c",
Line: 242)

Yes, the problem here is that dirty buffers can't look up the proper
relation to figure out how to complete the transaction. Reloading the
cache after the swap seems to fix it, but the invalidation itself throws
WARNINGS because the relfilenode's of the old and new relations
structures don't match, and when it tries to delete the first one, it
throws the warning.

(I think it's line 241 in pristine source). I tried doing
CatalogIndexInsert and CommandCounterIncrement after each
simple_heap_update, but the result is the same. This assertion is
failed at transaction commit, when LocalBufferSync is called.

Yep, I think everything now points to changes needed in relcache.c to
handle the new condition of a relation swapping its relnode. I looked
at the REINDEX code and it doesn't seem to have this problem. Not sure
why.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Tom Lane wrote:

This is particularly
significant for thisxactonly relations.

Yes. After thinking awhile I realize that the real problem is that we
are trying to swap between an existing relation (!rd_myxactonly) and
a new relation (rd_myxactonly). Buffers for one live in the main
buffer pool, for the other in the local buffer pool. There's also the
little matter of the local state inside relcache.c. While the update
to pg_class should make the right things happen to relfilenode, it
doesn't do anything to cause a change in rd_myxactonly status.

Yes, I had one intermediate patch where I was calling
RelationClearRelation directly and not using the reload flag computed by
RelationForgetRelation. Then I found RelationFlushRelation and that
seemed to work better.

This case is actually more complicated because after the swap, pg_class
still says thisxactonly but the relfilenode points to non-thisactonly
buffers, and via versa. One of the key things is that thisxactonly
relations can't be forgotten because there are local buffers associated
with the relation, or something like that.

--
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000   
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania
19026

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Yep, I think everything now points to changes needed in relcache.c to
handle the new condition of a relation swapping its relnode.

I still don't believe that any changes are needed in relcache.c.
In particular, after thinking more I see that we do not need to change
the myxactonly status of either relation: the temp one is still temp,
the original one is still not.

What we do need to do is to flush all buffers for the two rels from the
local and global buffer caches respectively, since after the swap they
should get buffered in different places. FlushRelationBuffers looks
like it will work for that.

regards, tom lane

Bruce Momjian <pgman@candle.pha.pa.us> writes:

This case is actually more complicated because after the swap, pg_class
still says thisxactonly but the relfilenode points to non-thisactonly
buffers, and via versa.

pg_class doesn't say anything (if it did, we could make relcache.c
track it).

But yes, the problem is that myxactonly has to line up with where the
buffers are stored. As I mentioned, I think it would work to flush out
all buffers for both rels before doing the swap.

regards, tom lane

Tom Lane dijo:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

This case is actually more complicated because after the swap, pg_class
still says thisxactonly but the relfilenode points to non-thisactonly
buffers, and via versa.

pg_class doesn't say anything (if it did, we could make relcache.c
track it).

But yes, the problem is that myxactonly has to line up with where the
buffers are stored. As I mentioned, I think it would work to flush out
all buffers for both rels before doing the swap.

It does indeed work. Actually, I tried to do that before, but the fact
that RelationFlushBuffers requires a firstDelBlock made me turn around.

I attach a new patch against current cvs HEAD. It works as far as I've
tested it, but I may be missing some cases. I'll post a documentation
patch later if this gets applied.

--
Alvaro Herrera (<alvherre[a]atentus.com>)
"Some men are heterosexual, and some are bisexual, and some
men don't think about sex at all... they become lawyers" (Woody Allen)

Alvaro Herrera <alvherre@atentus.com> writes:

It does indeed work. Actually, I tried to do that before, but the fact
that RelationFlushBuffers requires a firstDelBlock made me turn around.

firstDelBlock should be 0, not length-of-relation; as given, this code
fails to get rid of the buffer entries!

regards, tom lane

Tom Lane dijo:

Alvaro Herrera <alvherre@atentus.com> writes:

It does indeed work. Actually, I tried to do that before, but the fact
that RelationFlushBuffers requires a firstDelBlock made me turn around.

firstDelBlock should be 0, not length-of-relation; as given, this code
fails to get rid of the buffer entries!

Oh, I failed to understand completely the purpose of firstDelBlock then.

Anyway, there's still a big problem with this patch: the pg_depend
information gets messed up after CLUSTER, so a clustered table cannot be
dropped. I don't know why is this.

As I said yesterday, I'm going on vacation tomorrow, and probably will
not fix this issue. I can look into it when I'm back, on two weeks.

I attach a little doc patch.

--
Alvaro Herrera (<alvherre[a]atentus.com>)
"La gente vulgar solo piensa en pasar el tiempo;
el que tiene talento, en aprovecharlo"

Alvaro Herrera wrote:

Tom Lane dijo:

Alvaro Herrera <alvherre@atentus.com> writes:

It does indeed work. Actually, I tried to do that before, but the fact
that RelationFlushBuffers requires a firstDelBlock made me turn around.

firstDelBlock should be 0, not length-of-relation; as given, this code
fails to get rid of the buffer entries!

Oh, I failed to understand completely the purpose of firstDelBlock then.

Anyway, there's still a big problem with this patch: the pg_depend
information gets messed up after CLUSTER, so a clustered table cannot be
dropped. I don't know why is this.

I actually backed out Tom's recent change to cluster.c (attached),
applied your patch, then manually applied Tom's patch to cluster.c so I
had a working version of your patch with the new dependencies. Seemed
to work fine. I will do the same when I apply you version.

As I said yesterday, I'm going on vacation tomorrow, and probably will
not fix this issue. I can look into it when I'm back, on two weeks.

I will take care of applying your patch and the doc part too. It will
be all done when you return. Thanks for your work and have a nice
vacation.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Anyway, there's still a big problem with this patch: the pg_depend
information gets messed up after CLUSTER, so a clustered table cannot be
dropped. I don't know why is this.

I actually backed out Tom's recent change to cluster.c (attached),
applied your patch, then manually applied Tom's patch to cluster.c so I
had a working version of your patch with the new dependencies. Seemed
to work fine.

The changes I made to cluster.c may or may not be correct in the
context of a redone CLUSTER implementation; it'll need to be looked at.

regards, tom lane

Tom Lane wrote:

What we do need to do is to flush all buffers for the two rels from the
local and global buffer caches respectively, since after the swap they
should get buffered in different places. FlushRelationBuffers looks
like it will work for that.

Sure, if you want to take the easy solution. ;-)

Good idea, just flush the buffers so we don't have to track the change.

Thanks for the assistance, Tom. Getting a cluster that doesn't have
embarrassing shortcomings is a major improvement for 7.3.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Anyway, there's still a big problem with this patch: the pg_depend
information gets messed up after CLUSTER, so a clustered table cannot be
dropped. I don't know why is this.

I actually backed out Tom's recent change to cluster.c (attached),
applied your patch, then manually applied Tom's patch to cluster.c so I
had a working version of your patch with the new dependencies. Seemed
to work fine.

The changes I made to cluster.c may or may not be correct in the
context of a redone CLUSTER implementation; it'll need to be looked at.

Tom, you are probably right because the table being dropped doesn't have
anything associated with it, except that it has the same tuple
descriptor as the base table. Wonder if that is going to create things
like defaults that need to be cascade deleted:

OIDNewHeap = heap_create_with_catalog(NewName,
RelationGetNamespace(OldHeap),
tupdesc,
OldHeap->rd_rel->relkind,
OldHeap->rd_rel->relisshared,
OldHeap->rd_rel->relhasoids,
allowSystemTableMods);

If you can tell me which drop type is correct, I can apply the cluster patch.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Your patch has been added to the PostgreSQL unapplied patches list at:

http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

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

Alvaro Herrera wrote:

Tom Lane dijo:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

This case is actually more complicated because after the swap, pg_class
still says thisxactonly but the relfilenode points to non-thisactonly
buffers, and via versa.

pg_class doesn't say anything (if it did, we could make relcache.c
track it).

But yes, the problem is that myxactonly has to line up with where the
buffers are stored. As I mentioned, I think it would work to flush out
all buffers for both rels before doing the swap.

It does indeed work. Actually, I tried to do that before, but the fact
that RelationFlushBuffers requires a firstDelBlock made me turn around.

I attach a new patch against current cvs HEAD. It works as far as I've
tested it, but I may be missing some cases. I'll post a documentation
patch later if this gets applied.

--
Alvaro Herrera (<alvherre[a]atentus.com>)
"Some men are heterosexual, and some are bisexual, and some
men don't think about sex at all... they become lawyers" (Woody Allen)

Content-Description:

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 5: Have you checked our extensive FAQ?

http://www.postgresql.org/users-lounge/docs/faq.html

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026

Your patch has been added to the PostgreSQL unapplied patches list at:

http://candle.pha.pa.us/cgi-bin/pgpatches

I will try to apply it within the next 48 hours.

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

Alvaro Herrera wrote:

Tom Lane dijo:

Alvaro Herrera <alvherre@atentus.com> writes:

It does indeed work. Actually, I tried to do that before, but the fact
that RelationFlushBuffers requires a firstDelBlock made me turn around.

firstDelBlock should be 0, not length-of-relation; as given, this code
fails to get rid of the buffer entries!

Oh, I failed to understand completely the purpose of firstDelBlock then.

Anyway, there's still a big problem with this patch: the pg_depend
information gets messed up after CLUSTER, so a clustered table cannot be
dropped. I don't know why is this.

As I said yesterday, I'm going on vacation tomorrow, and probably will
not fix this issue. I can look into it when I'm back, on two weeks.

I attach a little doc patch.

--
Alvaro Herrera (<alvherre[a]atentus.com>)
"La gente vulgar solo piensa en pasar el tiempo;
el que tiene talento, en aprovecharlo"

Content-Description:

[ Attachment, skipping... ]

---------------------------(end of broadcast)---------------------------
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to majordomo@postgresql.org so that your
message can get through to the mailing list cleanly

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 853-3000
  +  If your life is a hard drive,     |  830 Blythe Avenue
  +  Christ can be your backup.        |  Drexel Hill, Pennsylvania 19026