CVSROOT:	/cvsroot
Module name:	pgsql-server
Changes by:	tgl@svr1.postgresql.org	03/09/19 16:57:42

Modified files:
	src/backend/catalog: index.c 
	src/backend/commands: indexcmds.c 

Log message:
	Seems like a bad idea that REINDEX TABLE supports (or thinks it does)
	reindexing system tables without ignoring system indexes, when the
	other two varieties of REINDEX disallow it.  Make all three act the same,
	and simplify downstream code accordingly.


Fri, 19 Sep 2003 16:57:42 -0300 (ADT)
tgl@svr1.postgresql.org (Tom Lane)
pgsql-server/src/backend catalog/index.c comma ...
> -----Original Message-----
> From: Tom Lane
> 
> CVSROOT:	/cvsroot
> Module name:	pgsql-server
> Changes by:	tgl@svr1.postgresql.org	03/09/19 16:57:42
> 
> Modified files:
> 	src/backend/catalog: index.c 
> 	src/backend/commands: indexcmds.c 
> 
> Log message:
> 	Seems like a bad idea that REINDEX TABLE supports (or 
> thinks it does)
> 	reindexing system tables without ignoring system 
> indexes, 

Why ?

regards.
Hiroshi Inoue


Sat, 20 Sep 2003 08:06:55 +0900
"Hiroshi Inoue" <inoue@tpf.co.jp>
Re: pgsql-server/src/backend catalog/index.c comma ...
"Hiroshi Inoue" <inoue@tpf.co.jp> writes:
>> Log message:
>> Seems like a bad idea that REINDEX TABLE supports (or 
>> thinks it does)
>> reindexing system tables without ignoring system 
>> indexes, 

> Why ?

I'd ask the question the other way: why would it be a good idea to allow
this in REINDEX TABLE and not in the other two cases?  And did it really
work?

The REINDEX code is messy and fragile enough that I think we should do
whatever we can to simplify it.

			regards, tom lane

Fri, 19 Sep 2003 19:31:27 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pgsql-server/src/backend catalog/index.c comma ... 
> 
> "Hiroshi Inoue" <inoue@tpf.co.jp> writes:
> >> Log message:
> >> Seems like a bad idea that REINDEX TABLE supports (or 
> >> thinks it does)
> >> reindexing system tables without ignoring system 
> >> indexes, 
> 
> > Why ?
> 
> I'd ask the question the other way: why would it be a good 
> idea to allow
> this in REINDEX TABLE and not in the other two cases?  And 
> did it really
> work?

Yes. I would revert your change.

regards,
Hiroshi Inoue


Sat, 20 Sep 2003 08:38:42 +0900
"Hiroshi Inoue" <inoue@tpf.co.jp>
Re: pgsql-server/src/backend catalog/index.c comma ... 
"Hiroshi Inoue" <inoue@tpf.co.jp> writes:
>> I'd ask the question the other way: why would it be a good 
>> idea to allow
>> this in REINDEX TABLE and not in the other two cases?  And 
>> did it really
>> work?

> Yes. I would revert your change.

You didn't answer the first question: why's this a good idea?
It seems risky and of little value to try to support system
table reindexing without disabling system indexes.

Also, your assertion that it works doesn't convince me.  That business
in reindex_table about doing two setRelhasindex() calls gave me the
willies.  Why was that needed?  "to keep consistency with WAL" isn't
enough commentary for code as strange as that.  And having a
CommandCounterIncrement() that's executed in some cases and not others
is a recipe for bugs; we've been burnt by that before.

			regards, tom lane

Fri, 19 Sep 2003 19:57:31 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pgsql-server/src/backend catalog/index.c comma ... 
> "Hiroshi Inoue" <inoue@tpf.co.jp> writes:
> >> I'd ask the question the other way: why would it be a good 
> >> idea to allow
> >> this in REINDEX TABLE and not in the other two cases?  And 
> >> did it really
> >> work?
> 
> > Yes. I would revert your change.
> 
> You didn't answer the first question: why's this a good idea?
> It seems risky and


>  of little value to try to support system
> table reindexing without disabling system indexes.

Why could you determine it ? Is PostgreSQL your system ?
Because It is never of little value of cource, I supported it.
I intended to support on-line REINDEX from the first, I first
implemented REINDEX in standalone mode not in bootstrap(Jan's
idea) mode. I also intended to support on-line reindexing nailed
relations but I didn't have time to achive it.
 
> Also, your assertion that it works doesn't convince me.  That business
> in reindex_table about doing two setRelhasindex() calls gave me the
> willies.  Why was that needed?

The setRelhasIndex() has no meaning to other backends, i.e the false state
is never visible to other backends. 

>  "to keep consistency with WAL" isn't
> enough commentary for code as strange as that.  And having a
> CommandCounterIncrement() that's executed in some cases and not others
> is a recipe for bugs; we've been burnt by that before.

The code you are referring is to reindex pg_class relation. The code has
never
active unless an #ifdef is defined.

regards,
Hiroshi Inoue


Sat, 20 Sep 2003 12:04:40 +0900
"Hiroshi Inoue" <inoue@tpf.co.jp>
Re: pgsql-server/src/backend catalog/index.c comma ... 
"Hiroshi Inoue" <inoue@tpf.co.jp> writes:
> Why could you determine it ? Is PostgreSQL your system ?

Well, if you prefer, we can have a discussion and vote about it on pghackers.
But you have not answered my questions.  Why was the code set up to
allow live reindexing of system tables via REINDEX TABLE, but not via
REINDEX INDEX or REINDEX DATABASE?  It seems to me that those three
cases ought to work equally well or badly.  Let's remove the restriction
from all three, if it's not needed.

>> Also, your assertion that it works doesn't convince me.  That business
>> in reindex_table about doing two setRelhasindex() calls gave me the
>> willies.  Why was that needed?

> The setRelhasIndex() has no meaning to other backends, i.e the false state
> is never visible to other backends. 

Then why bother?  I agree that it's probably not needed for other
backends --- since you have an exclusive lock on the table, nobody else
should be trying to examine it.  But then why do it at all?  The only
thing you need is to force IsIgnoringSystemIndexes true locally, which
would seem to me a much more secure way of proceeding.

The goal that I am trying to accomplish here is to make the reindexing
code more visibly safe and correct.  I'm not convinced that we still
need live-reindexing capability in 7.4 (I think the btree space
recycling code solves the problem better).  But I'm happy to leave it in
if these other questions can be addressed.

			regards, tom lane

Sat, 20 Sep 2003 11:37:32 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pgsql-server/src/backend catalog/index.c comma ... 
> "Hiroshi Inoue" <inoue@tpf.co.jp> writes:
> > Why could you determine it ? Is PostgreSQL your system ?
> 
> Well, if you prefer, we can have a discussion and vote about 
> it on pghackers.

Oh discussion *first* is good but You committed *first*.
So isn't it reasonable to revert your change *first* ?

This is the second time you disable the on-line reindex
functionality for system tables. Why must I explain the
same thing many times ?

May I ask again ? Is PostgreSQL your system ?

regards,
Hiroshi Inoue 


Sun, 21 Sep 2003 06:28:07 +0900
"Hiroshi Inoue" <inoue@tpf.co.jp>
Re: pgsql-server/src/backend catalog/index.c comma ... 


On Sun, 21 Sep 2003, Hiroshi Inoue wrote:

> > "Hiroshi Inoue" <inoue@tpf.co.jp> writes:
> > > Why could you determine it ? Is PostgreSQL your system ?
> >
> > Well, if you prefer, we can have a discussion and vote about
> > it on pghackers.
>
> Oh discussion *first* is good but You committed *first*.
> So isn't it reasonable to revert your change *first* ?
>
> This is the second time you disable the on-line reindex
> functionality for system tables. Why must I explain the
> same thing many times ?

Actually, as a comment here, since I *think* I understand where Tom is
coming from ... and since I've either missed it, or it hasn't been
answered yet ... why was the original patch incomplete in only addressing
1 of 3 REINDEX conditions?  Is there a reason why that one condition
is/was safe to do it with, but not the other 2?

Again ... if I understand Tom's objections to, and reason for reversing,
this patch correctly ...

Sat, 20 Sep 2003 18:45:24 -0300 (ADT)
"Marc G. Fournier" <scrappy@postgresql.org>
Re: pgsql-server/src/backend catalog/index.c comma ...
"Marc G. Fournier" <scrappy@postgresql.org> writes:
> Actually, as a comment here, since I *think* I understand where Tom is
> coming from ... and since I've either missed it, or it hasn't been
> answered yet ... why was the original patch incomplete in only addressing
> 1 of 3 REINDEX conditions?  Is there a reason why that one condition
> is/was safe to do it with, but not the other 2?

That's exactly what's bothering me.  Where I'd like to end up is that
either all three variants of REINDEX allow this, or all three do not.
I don't understand why only REINDEX TABLE should support it.

			regards, tom lane

Sat, 20 Sep 2003 18:06:20 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pgsql-server/src/backend catalog/index.c comma ... 
> -----Original Message-----
> From: Marc G. Fournier [mailto:scrappy@postgresql.org] 
> Sent: Sunday, September 21, 2003 6:45 AM
> To: Hiroshi Inoue
> Cc: 'Tom Lane'; pgsql-committers@postgresql.org
> Subject: Re: [COMMITTERS] pgsql-server/src/backend 
> catalog/index.c comma ...
> 
> On Sun, 21 Sep 2003, Hiroshi Inoue wrote:
> 
> > > "Hiroshi Inoue" <inoue@tpf.co.jp> writes:
> > > > Why could you determine it ? Is PostgreSQL your system ?
> > >
> > > Well, if you prefer, we can have a discussion and vote about
> > > it on pghackers.
> >
> > Oh discussion *first* is good but You committed *first*.
> > So isn't it reasonable to revert your change *first* ?
> >
> > This is the second time you disable the on-line reindex
> > functionality for system tables. Why must I explain the
> > same thing many times ?
> 
> Actually, as a comment here, since I *think* I understand where Tom is
> coming from ... and since I've either missed it, or it hasn't been
> answered yet ... why was the original patch incomplete in 
> only addressing
> 1 of 3 REINDEX conditions?  Is there a reason why that one condition
> is/was safe to do it with, but not the other 2?

Sorry to trouble you.

In the first place, REINDEX is neither an SQL standard command nor
a preferable one.

When I introduced REINDEX command before 7.0, it was not
transaction-safe and only allowed to call under standalone mode
essentially.

Before 7.1, the introduction of pg_class.relfilenode gave us a possibilty
to make REINDEX command transaction-safe and I tried to make
REINDEX available under postmaster and the result was
   1.All user indexes/tables could be REINDEXed under postmaster
   2.System tables except shared or nailed ones could be REINDEXed
     under postmaster.

Note that we couldn't reindex all system tables under postmaster.
It's the main reason why I didn't implement REINDEX DATABASE
under postmaster. As for REINDEX, I have 

Tue Nov 20 02:46:13 2001 UTC (22 months ago)  Tom committed
the following change which disables the functionality to reindex
system tables under postmaster.
     Some minor tweaks of REINDEX processing: grab exclusive
     lock a little earlier, make error checks more uniform.

The above change was the first time he disables the functionality. 
I noticed the change, complained to him and
Thu Feb 7 00:27:30 2002 UTC (19 months, 1 week ago) I  
   Removed a check for REINDEX TABLE.

And this is the second time he disables the functionality without
any notification to me. Honestly I don't understand why I have to
explain this kind of thing only so as to revert the change.

regards,
Hiroshi Inoue
 


Sun, 21 Sep 2003 20:04:43 +0900
"Hiroshi Inoue" <inoue@tpf.co.jp>
Re: pgsql-server/src/backend catalog/index.c comma ...
I've been looking at the issues involved in reindexing system tables,
and I now have what I think is a fairly defensible set of proposals.

We should whenever possible use the same reindexing technique used by
CLUSTER: assign a new relfilenode number, build the new index in that
file, and apply an ordinary heap_update operation to update the index's
pg_class row with the new relfilenode value.  This technique is fully
crash-safe and transaction-safe (meaning it can be rolled back even after
completion, in case of failure later in the same transaction).  However,
there are several pitfalls for applying it to system indexes:

1. There is a problem for a shared index, because we have no way to
update pg_class.relfilenode in all the other databases besides ours.
I see no good way around this problem.

2. There is a problem for a nailed-in-cache index, because the relcache
isn't prepared to cope with relfilenode updates for such indexes.
However, that is fixable.

3. There is a problem for an index on pg_class itself: doing heap_update
on a pg_class row must make new index entries.  We have to be careful
that the new row does appear in the updated index, while not making
entries in old-and-possibly-broken indexes.  This is doable.

4. There is a problem with updating indexes that might be used for catalog
fetches executed during the index_build process: if we try to use the
partially-rebuilt index for such a fetch we will fail.  In the case of
disaster recovery the problem doesn't exist because we'll have
IsIgnoringSystemIndexes true, but for an ordinary on-line indexing
operation there's definitely a risk.  Presently the code relies on
"deactivating indexes" to prevent this, but I think it can be done more
simply, because we only need to suppress access to the target index
locally in the reindexing backend.  (Other backends will be locked out by
means of an AccessExclusiveLock on the system catalog in question.)

In short then, we can fix things so that the new-relfilenode path can be
taken in all cases except shared indexes.  For shared indexes, we have
little choice but to truncate and rebuild the index in-place.  (There is
then no need to update its pg_class row at all.)  This is of course
horribly crash-unsafe.

The code presently uses a "deactivated indexes" flag (namely, setting
pg_class.relhasindex false) to provide some protection against a crash
midway through an in-place reindex.  However, this concept is really quite
useless for a shared index, because only one database could contain the
deactivation flag.  Accesses from any other database would still try to
use the broken index.

Based on this analysis, I feel that the "deactivated indexes" mechanism
is of no further value and should be retired.  We should instead simply
acknowledge that reindexing shared indexes is dangerous.  I propose that
it be allowed only from a standalone backend.  Specifically I think that
REINDEX INDEX and REINDEX TABLE should error out if pointed to a shared
index or table, unless running in a standalone backend; REINDEX DATABASE
should skip over the shared indexes unless running in a standalone
backend.  (I'm not convinced that either -O or -P needs to be insisted on
by REINDEX, btw, but we can discuss that separately.)  We'll have to warn
users not to try to restart the database when reindexing of a shared table
wasn't successfully completed.

Details to back up the above claims:

Part of the code needed to fix the relcache restriction on nailed indexes
is already there, but ifdef'd out; that's the part that re-reads the
index's pg_class row after receiving SI inval for it.  There are some
cases not yet handled though.  In the first place, if the nailed index
being modified is pg_class_oid_index, ScanPgRelation will try to use that
same index to load the updated row, and will fail because it'll be trying
to use the old relfilenode.  We can easily force a seqscan in that case,
however.  A more subtle problem is that if an SI buffer overflow occurs, 
RelationCacheInvalidate walks through the relation cache in a random
order.  I believe that the correct update order has to be first
pg_class_oid_index, then the other nailed indexes, and finally all other
relation cache entries.  pg_class_oid_index has to be updated first (with
the aforementioned seqscan), since it will be used by ScanPgRelation to
reread the pg_class rows for the other nailed indexes.  Only when we are
sure the nailed indexes are up-to-date can we safely rebuild other
relcache entries.

Assigning a new relfilenode to indexes of pg_class is not a big deal when
we don't have an index-corruption problem.  We simply have to make the new
heap_updated row before we start index_build (which indeed the code does
already); both old and new rows will be indexed in the new index, and all
is well.  However, if we suspect index corruption then it is important not
to try to make entries into the old indexes, because we might crash trying
to update a corrupt index.  I propose the following behavior:

	REINDEX INDEX: no special action; this is not the thing to use
	when index corruption is suspected anyway.

	REINDEX TABLE: while operating on pg_class, arrange for the
	relcache's list of known indexes of pg_class to contain only the
	indexes already reindexed.  In this way, only those indexes will
	be updated by CatalogUpdateIndexes(), and so no untrustworthy
	indexes will be touched while making new pg_class rows.

	REINDEX DATABASE: take care to process pg_class first.

To avoid catalog accesses via an index that's being rebuilt, we can simply
generalize the SetReindexProcessing() state to include the OID of the
index currently being rebuilt.  Places that presently make checks for
IsIgnoringSystemIndexes can check this as well and fall back to seqscans
when the targeted index would have been used.

One other point: IsIgnoringSystemIndexes seems presently to be taken to
mean not only that we don't *read* the system indexes, but that we don't
*write* them either.  I think this is horribly dangerous; it effectively
means that the only thing you can safely do in a backend started with -P
is REINDEX.  If you make any other changes to the system catalogs then
you've re-corrupted the indexes.  Also, you don't have any protection
against duplicate entries getting made, since the unique indexes are
disabled.  It would be a lot safer to define IsIgnoringSystemIndexes as
preventing the system indexes from being used for lookups, while still
updating the indexes on any catalog modification.  This will not affect
the safety of REINDEX and will make other operations much less prone to
pilot error.

Comments?

			regards, tom lane

Sun, 21 Sep 2003 14:55:01 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Improving REINDEX for system indexes (long)
First it should have been discussed before your commitment or at least
it should be discussed after reversing your change.

I require you to explain me why you committed the change
with no discussion and little investigation.

I also noticed that your change for catalog/index.c
 
    Revision 1.200 / (download) - annotate - [select for diffs] , Mon Sep 23
00:42:48 2002 UTC (11 months, 4 weeks ago) by tgl 
    Changes since 1.199: +124 -161 lines
    Diff to previous 1.199 
    Get rid of bogus use of heap_mark4update in reindex operations (cf.
    recent bug report).  Fix processing of nailed-in-cache indexes;
    it appears that REINDEX DATABASE has been broken for months :-(.

removed an #ifndef ENABLE_REINDEX_NAILED_RELATIONS
trial stuff. I'm very surprised to see it now.

regards,
Hiroshi Inoue

> -----Original Message-----
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us] 
> 
> I've been looking at the issues involved in reindexing system tables,
> and I now have what I think is a fairly defensible set of proposals.
> 
> We should whenever possible use the same reindexing technique used by
> CLUSTER: assign a new relfilenode number, build the new index in that
> file, and apply an ordinary heap_update operation to update 
> the index's
> pg_class row with the new relfilenode value.  This technique is fully
> crash-safe and transaction-safe (meaning it can be rolled 
> back even after
> completion, in case of failure later in the same 
> transaction).  However,
> there are several pitfalls for applying it to system indexes:
> 
> 1. There is a problem for a shared index, because we have no way to
> update pg_class.relfilenode in all the other databases besides ours.
> I see no good way around this problem.
> 
> 2. There is a problem for a nailed-in-cache index, because 
> the relcache
> isn't prepared to cope with relfilenode updates for such indexes.
> However, that is fixable.
> 
> 3. There is a problem for an index on pg_class itself: doing 
> heap_update
> on a pg_class row must make new index entries.  We have to be careful
> that the new row does appear in the updated index, while not making
> entries in old-and-possibly-broken indexes.  This is doable.
> 
> 4. There is a problem with updating indexes that might be 
> used for catalog
> fetches executed during the index_build process: if we try to use the
> partially-rebuilt index for such a fetch we will fail.  In the case of
> disaster recovery the problem doesn't exist because we'll have
> IsIgnoringSystemIndexes true, but for an ordinary on-line indexing
> operation there's definitely a risk.  Presently the code relies on
> "deactivating indexes" to prevent this, but I think it can be 
> done more
> simply, because we only need to suppress access to the target index
> locally in the reindexing backend.  (Other backends will be 
> locked out by
> means of an AccessExclusiveLock on the system catalog in question.)
> 
> In short then, we can fix things so that the new-relfilenode 
> path can be
> taken in all cases except shared indexes.  For shared indexes, we have
> little choice but to truncate and rebuild the index in-place. 
>  (There is
> then no need to update its pg_class row at all.)  This is of course
> horribly crash-unsafe.
> 
> The code presently uses a "deactivated indexes" flag (namely, setting
> pg_class.relhasindex false) to provide some protection against a crash
> midway through an in-place reindex.  However, this concept is 
> really quite
> useless for a shared index, because only one database could 
> contain the
> deactivation flag.  Accesses from any other database would 
> still try to
> use the broken index.
> 
> Based on this analysis, I feel that the "deactivated indexes" 
> mechanism
> is of no further value and should be retired.  We should 
> instead simply
> acknowledge that reindexing shared indexes is dangerous.  I 
> propose that
> it be allowed only from a standalone backend.  Specifically I 
> think that
> REINDEX INDEX and REINDEX TABLE should error out if pointed 
> to a shared
> index or table, unless running in a standalone backend; 
> REINDEX DATABASE
> should skip over the shared indexes unless running in a standalone
> backend.  (I'm not convinced that either -O or -P needs to be 
> insisted on
> by REINDEX, btw, but we can discuss that separately.)  We'll 
> have to warn
> users not to try to restart the database when reindexing of a 
> shared table
> wasn't successfully completed.
> 
> Details to back up the above claims:
> 
> Part of the code needed to fix the relcache restriction on 
> nailed indexes
> is already there, but ifdef'd out; that's the part that re-reads the
> index's pg_class row after receiving SI inval for it.  There are some
> cases not yet handled though.  In the first place, if the nailed index
> being modified is pg_class_oid_index, ScanPgRelation will try 
> to use that
> same index to load the updated row, and will fail because 
> it'll be trying
> to use the old relfilenode.  We can easily force a seqscan in 
> that case,
> however.  A more subtle problem is that if an SI buffer 
> overflow occurs, 
> RelationCacheInvalidate walks through the relation cache in a random
> order.  I believe that the correct update order has to be first
> pg_class_oid_index, then the other nailed indexes, and 
> finally all other
> relation cache entries.  pg_class_oid_index has to be updated 
> first (with
> the aforementioned seqscan), since it will be used by 
> ScanPgRelation to
> reread the pg_class rows for the other nailed indexes.  Only 
> when we are
> sure the nailed indexes are up-to-date can we safely rebuild other
> relcache entries.
> 
> Assigning a new relfilenode to indexes of pg_class is not a 
> big deal when
> we don't have an index-corruption problem.  We simply have to 
> make the new
> heap_updated row before we start index_build (which indeed 
> the code does
> already); both old and new rows will be indexed in the new 
> index, and all
> is well.  However, if we suspect index corruption then it is 
> important not
> to try to make entries into the old indexes, because we might 
> crash trying
> to update a corrupt index.  I propose the following behavior:
> 
> 	REINDEX INDEX: no special action; this is not the thing to use
> 	when index corruption is suspected anyway.
> 
> 	REINDEX TABLE: while operating on pg_class, arrange for the
> 	relcache's list of known indexes of pg_class to contain only the
> 	indexes already reindexed.  In this way, only those indexes will
> 	be updated by CatalogUpdateIndexes(), and so no untrustworthy
> 	indexes will be touched while making new pg_class rows.
> 
> 	REINDEX DATABASE: take care to process pg_class first.
> 
> To avoid catalog accesses via an index that's being rebuilt, 
> we can simply
> generalize the SetReindexProcessing() state to include the OID of the
> index currently being rebuilt.  Places that presently make checks for
> IsIgnoringSystemIndexes can check this as well and fall back 
> to seqscans
> when the targeted index would have been used.
> 
> One other point: IsIgnoringSystemIndexes seems presently to 
> be taken to
> mean not only that we don't *read* the system indexes, but 
> that we don't
> *write* them either.  I think this is horribly dangerous; it 
> effectively
> means that the only thing you can safely do in a backend 
> started with -P
> is REINDEX.  If you make any other changes to the system catalogs then
> you've re-corrupted the indexes.  Also, you don't have any protection
> against duplicate entries getting made, since the unique indexes are
> disabled.  It would be a lot safer to define 
> IsIgnoringSystemIndexes as
> preventing the system indexes from being used for lookups, while still
> updating the indexes on any catalog modification.  This will 
> not affect
> the safety of REINDEX and will make other operations much 
> less prone to
> pilot error.
> 
> Comments?
> 
> 			regards, tom lane
> 


Mon, 22 Sep 2003 04:56:35 +0900
"Hiroshi Inoue" <inoue@tpf.co.jp>
Re: Improving REINDEX for system indexes (long)
"Hiroshi Inoue" <inoue@tpf.co.jp> writes:
> I require you to explain me why you committed the change
> with no discussion and little investigation.

If you want an apology for not having discussed it in advance, I'll
gladly offer one.  It was poorly done.

I do, however, think that the reindexing code needs work.  Can we
get on with discussing the technical issues?

			regards, tom lane

Sun, 21 Sep 2003 16:04:34 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Improving REINDEX for system indexes (long) 
On Mon, Sep 22, 2003 at 04:56:35AM +0900, Hiroshi Inoue wrote:
> First it should have been discussed before your commitment or at least
> it should be discussed after reversing your change.
> 
> I require you to explain me why you committed the change
> with no discussion and little investigation.
> 
> I also noticed that your change for catalog/index.c
>  
>     Revision 1.200 / (download) - annotate - [select for diffs] , Mon Sep 23
> 00:42:48 2002 UTC (11 months, 4 weeks ago) by tgl 

You know you're talking about a commit of 1 year ago?


Kurt


Mon, 22 Sep 2003 00:34:15 +0200
Kurt Roeckx <Q@ping.be>
Re: Improving REINDEX for system indexes (long)
Kurt Roeckx wrote:
> 
> On Mon, Sep 22, 2003 at 04:56:35AM +0900, Hiroshi Inoue wrote:
> > First it should have been discussed before your commitment or at least
> > it should be discussed after reversing your change.
> >
> > I require you to explain me why you committed the change
> > with no discussion and little investigation.
> >
> > I also noticed that your change for catalog/index.c
> >
> >     Revision 1.200 / (download) - annotate - [select for diffs] , Mon Sep 23
> > 00:42:48 2002 UTC (11 months, 4 weeks ago) by tgl
> 
> You know you're talking about a commit of 1 year ago?

Yes of cource. Unfortunately I haven't had enough time to
check all the changes since long. This time I was lucky to
find a change to spoil my effort. Is it strange for me to
doubt other places ? I looked again the current code about
REINDEX command and found the above change which is closely
related to this topic. 

regards,
Hiroshi Inoue
	http://www.geocities.jp/inocchichichi/psqlodbc/

Mon, 22 Sep 2003 09:53:38 +0900
Hiroshi Inoue <Inoue@tpf.co.jp>
Re: Improving REINDEX for system indexes (long)
Tom Lane wrote:
> 
> "Hiroshi Inoue" <inoue@tpf.co.jp> writes:
> > I require you to explain me why you committed the change
> > with no discussion and little investigation.
> 
> If you want an apology for not having discussed it in advance, I'll
> gladly offer one.  It was poorly done.

Thanks. 
> I do, however, think that the reindexing code needs work.  Can we
> get on with discussing the technical issues?

OK but I would need some time to remember the code.

regards,
Hiroshi Inoue
	http://www.geocities.jp/inocchichichi/psqlodbc/

Mon, 22 Sep 2003 13:21:42 +0900
Hiroshi Inoue <Inoue@tpf.co.jp>
Re: Improving REINDEX for system indexes (long)
Here is the proposed patch.  This fixes the problems that prevented the
relcache from coping with relfilenode changes for nailed-in-cache
indexes.  Accordingly, REINDEX is now crash-safe and transaction-safe
for all cases except reindexing shared system catalogs.  That case is
permitted only in a standalone backend (!IsUnderPostmaster).  We no
longer require -O to reindex system tables, and since -P now suppresses
only reads not writes of system indexes, it is safe to allow from the
client side.  Upshot: reindexing system catalogs can be done without a
standalone backend for all cases except shared catalogs.

			regards, tom lane


*** doc/src/sgml/ref/postgres-ref.sgml.orig	Thu Sep 18 16:30:15 2003
--- doc/src/sgml/ref/postgres-ref.sgml	Mon Sep 22 20:10:32 2003
***************
*** 177,185 ****
        <term><option>-P</option></term>
        <listitem>
         <para>
! 	Ignore system indexes while scanning/updating system tables. The
! 	<command>REINDEX</command> command for system tables/indexes
! 	requires this option to be used.
         </para>
        </listitem>
       </varlistentry>
--- 177,185 ----
        <term><option>-P</option></term>
        <listitem>
         <para>
! 	Ignore system indexes when reading system tables (but still update
! 	the indexes when modifying the tables).  This is useful when
! 	recovering from damaged system indexes.
         </para>
        </listitem>
       </varlistentry>
*** doc/src/sgml/ref/reindex.sgml.orig	Thu Sep 11 21:47:19 2003
--- doc/src/sgml/ref/reindex.sgml	Mon Sep 22 20:23:17 2003
***************
*** 56,98 ****
      </listitem>
     </itemizedlist>
    </para>
- 
-   <para>
-    If you suspect corruption of an index on a user table, you can
-    simply rebuild that index, or all indexes on the table, using
-    <command>REINDEX INDEX</command> or <command>REINDEX
-    TABLE</command>.  Another approach to dealing with a corrupted
-    user-table index is just to drop and recreate it.  This may in fact
-    be preferable if you would like to maintain some semblance of
-    normal operation on the table meanwhile.  <command>REINDEX</>
-    acquires exclusive lock on the table, while <command>CREATE
-    INDEX</> only locks out writes not reads of the table.
-   </para>
- 
-   <para>
-    Things are more difficult if you need to recover from corruption of
-    an index on a system table.  In this case it's important for the
-    system to not have used any of the suspect indexes itself.
-    (Indeed, in this sort of scenario you may find that server
-    processes are crashing immediately at start-up, due to reliance on
-    the corrupted indexes.)  To recover safely, the server must be shut
-    down and a stand-alone <productname>PostgreSQL</productname> server
-    must be started instead with the command-line options
-    <option>-O</option> and <option>-P</option>.  (These options allow
-    system table modifications and prevent use of system indexes,
-    respectively.)  Then, <command>REINDEX DATABASE</>,
-    <command>REINDEX TABLE</>, or <command>REINDEX INDEX</> can be
-    issued, depending on how much you want to reconstruct.  If in
-    doubt, use <command>REINDEX DATABASE FORCE</> to force
-    reconstruction of all system indexes in the database.  Then quit
-    the standalone server session and restart the real server.
-   </para>
- 
-   <para>
-    See the <xref linkend="app-postgres"> reference page for more
-    information about how to interact with the stand-alone server
-    interface.
-   </para>
   </refsect1>
    
   <refsect1>
--- 56,61 ----
***************
*** 104,111 ****
      <listitem>
       <para>
        Recreate all system indexes of a specified database. Indexes on
!       user tables are not included. This form of <command>REINDEX</>
!       can only be used in stand-alone mode (see above).
       </para>
      </listitem>
     </varlistentry>
--- 67,74 ----
      <listitem>
       <para>
        Recreate all system indexes of a specified database. Indexes on
!       user tables are not processed.  Also, indexes on shared system
!       catalogs are skipped except in stand-alone mode (see below).
       </para>
      </listitem>
     </varlistentry>
***************
*** 142,151 ****
      <term><literal>FORCE</literal></term>
      <listitem>
       <para>
!       Force rebuild of system indexes.  Without this key word,
!       <command>REINDEX</> skips system indexes that are not marked
!       invalid.  <literal>FORCE</> is irrelevant for <command>REINDEX
!       INDEX</> or when reindexing user indexes.
       </para>
      </listitem>
     </varlistentry>
--- 105,111 ----
      <term><literal>FORCE</literal></term>
      <listitem>
       <para>
!       This is an obsolete option; it is ignored if specified.
       </para>
      </listitem>
     </varlistentry>
***************
*** 153,158 ****
--- 113,190 ----
   </refsect1>
  
   <refsect1>
+   <title>Notes</title>
+ 
+   <para>
+    If you suspect corruption of an index on a user table, you can
+    simply rebuild that index, or all indexes on the table, using
+    <command>REINDEX INDEX</command> or <command>REINDEX
+    TABLE</command>.  Another approach to dealing with a corrupted
+    user-table index is just to drop and recreate it.  This may in fact
+    be preferable if you would like to maintain some semblance of
+    normal operation on the table meanwhile.  <command>REINDEX</>
+    acquires exclusive lock on the table, while <command>CREATE
+    INDEX</> only locks out writes not reads of the table.
+   </para>
+ 
+   <para>
+    Things are more difficult if you need to recover from corruption of
+    an index on a system table.  In this case it's important for the
+    system to not have used any of the suspect indexes itself.
+    (Indeed, in this sort of scenario you may find that server
+    processes are crashing immediately at start-up, due to reliance on
+    the corrupted indexes.)  To recover safely, the server must be started
+    with the <option>-P</option> option, which prevents it from using
+    indexes for system catalog lookups.
+   </para>
+ 
+   <para>
+    One way to do this is to shut down the postmaster and start a stand-alone
+    <productname>PostgreSQL</productname> server
+    with the <option>-P</option> option included on its command line.
+    Then, <command>REINDEX DATABASE</>,
+    <command>REINDEX TABLE</>, or <command>REINDEX INDEX</> can be
+    issued, depending on how much you want to reconstruct.  If in
+    doubt, use <command>REINDEX DATABASE</> to select
+    reconstruction of all system indexes in the database.  Then quit
+    the standalone server session and restart the regular server.
+    See the <xref linkend="app-postgres"> reference page for more
+    information about how to interact with the stand-alone server
+    interface.
+   </para>
+ 
+   <para>
+    Alternatively, a regular server session can be started with
+    <option>-P</option> included in its command line options.
+    The method for doing this varies across clients, but in all
+    <application>libpq</>-based clients, it is possible to set
+    the <envar>PGOPTIONS</envar> environment variable to <literal>-P</>
+    before starting the client.  Note that while this method does not
+    require locking out other clients, it may still be wise to prevent
+    other users from connecting to the damaged database until repairs
+    have been completed.
+   </para>
+ 
+   <para>
+    If corruption is suspected in the indexes of any of the shared
+    system catalogs (<structname>pg_database</structname>,
+    <structname>pg_group</structname>, or
+    <structname>pg_shadow</structname>), then a standalone server
+    must be used to repair it.  <command>REINDEX</> will not process
+    shared catalogs in multiuser mode.
+   </para>
+ 
+   <para>
+    For all indexes except the shared system catalogs, <command>REINDEX</>
+    is crash-safe and transaction-safe.  <command>REINDEX</> is not
+    crash-safe for shared indexes, which is why this case is disallowed
+    during normal operation.  If a failure occurs while reindexing one
+    of these catalogs in standalone mode, it is important that the failure
+    be rectified before attempting to restart the regular server.
+   </para>
+  </refsect1>
+ 
+  <refsect1>
    <title>Examples</title>
  
    <para>
***************
*** 172,182 ****
    </para>
  
    <para>
!    Rebuild all system indexes (this will only work in a stand-alone
!    server session):
  
  <programlisting>
! REINDEX DATABASE my_database FORCE;
  </programlisting>
    </para>
   </refsect1>
--- 204,218 ----
    </para>
  
    <para>
!    Rebuild all system indexes in a particular database, without trusting them
!    to be valid already:
  
  <programlisting>
! $ <userinput>export PGOPTIONS="-P"</userinput>
! $ <userinput>psql broken_db</userinput>
! ...
! broken_db=> REINDEX DATABASE broken_db;
! broken_db=> \q
  </programlisting>
    </para>
   </refsect1>
*** src/backend/access/index/genam.c.orig	Sun Aug  3 23:00:25 2003
--- src/backend/access/index/genam.c	Mon Sep 22 17:44:29 2003
***************
*** 184,204 ****
  				   int nkeys, ScanKey key)
  {
  	SysScanDesc sysscan;
  
  	sysscan = (SysScanDesc) palloc(sizeof(SysScanDescData));
  
  	sysscan->heap_rel = heapRelation;
  
! 	if (indexOK &&
! 		heapRelation->rd_rel->relhasindex &&
! 		!IsIgnoringSystemIndexes())
  	{
- 		Relation	irel;
  		int			i;
  
- 		/* We assume it's a system index, so index_openr is OK */
- 		sysscan->irel = irel = index_openr(indexRelname);
- 
  		/*
  		 * Change attribute numbers to be index column numbers.
  		 *
--- 184,215 ----
  				   int nkeys, ScanKey key)
  {
  	SysScanDesc sysscan;
+ 	Relation	irel;
+ 
+ 	if (indexOK && !IsIgnoringSystemIndexes())
+ 	{
+ 		/* We assume it's a system index, so index_openr is OK */
+ 		irel = index_openr(indexRelname);
+ 
+ 		if (ReindexIsProcessingIndex(RelationGetRelid(irel)))
+ 		{
+ 			/* oops, can't use index that's being rebuilt */
+ 			index_close(irel);
+ 			irel = NULL;
+ 		}
+ 	}
+ 	else
+ 		irel = NULL;
  
  	sysscan = (SysScanDesc) palloc(sizeof(SysScanDescData));
  
  	sysscan->heap_rel = heapRelation;
+ 	sysscan->irel = irel;
  
! 	if (irel)
  	{
  		int			i;
  
  		/*
  		 * Change attribute numbers to be index column numbers.
  		 *
***************
*** 210,222 ****
  			Assert(key[i].sk_attno == irel->rd_index->indkey[i]);
  			key[i].sk_attno = i + 1;
  		}
  		sysscan->iscan = index_beginscan(heapRelation, irel, snapshot,
  										 nkeys, key);
  		sysscan->scan = NULL;
  	}
  	else
  	{
- 		sysscan->irel = NULL;
  		sysscan->scan = heap_beginscan(heapRelation, snapshot, nkeys, key);
  		sysscan->iscan = NULL;
  	}
--- 221,233 ----
  			Assert(key[i].sk_attno == irel->rd_index->indkey[i]);
  			key[i].sk_attno = i + 1;
  		}
+ 
  		sysscan->iscan = index_beginscan(heapRelation, irel, snapshot,
  										 nkeys, key);
  		sysscan->scan = NULL;
  	}
  	else
  	{
  		sysscan->scan = heap_beginscan(heapRelation, snapshot, nkeys, key);
  		sysscan->iscan = NULL;
  	}
*** src/backend/access/transam/xact.c.orig	Fri Aug  8 17:47:02 2003
--- src/backend/access/transam/xact.c	Mon Sep 22 17:44:36 2003
***************
*** 834,841 ****
  	 */
  	s->state = TRANS_START;
  
- 	SetReindexProcessing(false);
- 
  	/*
  	 * generate a new transaction id
  	 */
--- 834,839 ----
***************
*** 1085,1090 ****
--- 1083,1089 ----
  	AtEOXact_Namespace(false);
  	AtEOXact_CatCache(false);
  	AtEOXact_Files();
+ 	SetReindexProcessing(InvalidOid, InvalidOid);
  	pgstat_count_xact_rollback();
  
  	/*
*** src/backend/catalog/index.c.orig	Fri Sep 19 15:57:42 2003
--- src/backend/catalog/index.c	Mon Sep 22 19:03:52 2003
***************
*** 78,101 ****
  static Oid	IndexGetRelation(Oid indexId);
  
  
- static bool reindexing = false;
- 
- 
- bool
- SetReindexProcessing(bool reindexmode)
- {
- 	bool		old = reindexing;
- 
- 	reindexing = reindexmode;
- 	return old;
- }
- 
- bool
- IsReindexProcessing(void)
- {
- 	return reindexing;
- }
- 
  /*
   *		ConstructTupleDescriptor
   *
--- 78,83 ----
***************
*** 497,504 ****
  	Oid			indexoid;
  	int			i;
  
- 	SetReindexProcessing(false);
- 
  	/*
  	 * Only SELECT ... FOR UPDATE are allowed while doing this
  	 */
--- 479,484 ----
***************
*** 972,1017 ****
  }
  
  
- /* ---------------------------------------------
-  *		Indexes of the relation active ?
-  *
-  * Caller must hold an adequate lock on the relation to ensure the
-  * answer won't be changing.
-  * ---------------------------------------------
-  */
- bool
- IndexesAreActive(Relation heaprel)
- {
- 	bool		isactive;
- 	Relation	indexRelation;
- 	HeapScanDesc scan;
- 	ScanKeyData entry;
- 
- 	if (heaprel->rd_rel->relkind != RELKIND_RELATION &&
- 		heaprel->rd_rel->relkind != RELKIND_TOASTVALUE)
- 		ereport(ERROR,
- 				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
- 				 errmsg("relation \"%s\" isn't an indexable relation",
- 						RelationGetRelationName(heaprel))));
- 
- 	/* If pg_class.relhasindex is set, indexes are active */
- 	isactive = heaprel->rd_rel->relhasindex;
- 	if (isactive)
- 		return isactive;
- 
- 	/* Otherwise, look to see if there are any indexes */
- 	indexRelation = heap_openr(IndexRelationName, AccessShareLock);
- 	ScanKeyEntryInitialize(&entry, 0,
- 						   Anum_pg_index_indrelid, F_OIDEQ,
- 						   ObjectIdGetDatum(RelationGetRelid(heaprel)));
- 	scan = heap_beginscan(indexRelation, SnapshotNow, 1, &entry);
- 	if (heap_getnext(scan, ForwardScanDirection) == NULL)
- 		isactive = true;		/* no indexes, so report "active" */
- 	heap_endscan(scan);
- 	heap_close(indexRelation, AccessShareLock);
- 	return isactive;
- }
- 
  /* ----------------
   *		set relhasindex of relation's pg_class entry
   *
--- 952,957 ----
***************
*** 1037,1048 ****
  	HeapScanDesc pg_class_scan = NULL;
  
  	/*
! 	 * Find the tuple to update in pg_class.
  	 */
  	pg_class = heap_openr(RelationRelationName, RowExclusiveLock);
  
! 	if (!IsIgnoringSystemIndexes() &&
! 		(!IsReindexProcessing() || pg_class->rd_rel->relhasindex))
  	{
  		tuple = SearchSysCacheCopy(RELOID,
  								   ObjectIdGetDatum(relid),
--- 977,989 ----
  	HeapScanDesc pg_class_scan = NULL;
  
  	/*
! 	 * Find the tuple to update in pg_class.  In bootstrap mode we can't
! 	 * use heap_update, so cheat and overwrite the tuple in-place.  In
! 	 * normal processing, make a copy to scribble on.
  	 */
  	pg_class = heap_openr(RelationRelationName, RowExclusiveLock);
  
! 	if (!IsBootstrapProcessingMode())
  	{
  		tuple = SearchSysCacheCopy(RELOID,
  								   ObjectIdGetDatum(relid),
***************
*** 1063,1077 ****
  
  	if (!HeapTupleIsValid(tuple))
  		elog(ERROR, "could not find tuple for relation %u", relid);
  
- 	/*
- 	 * Update fields in the pg_class tuple.
- 	 */
  	if (pg_class_scan)
  		LockBuffer(pg_class_scan->rs_cbuf, BUFFER_LOCK_EXCLUSIVE);
  
- 	classtuple = (Form_pg_class) GETSTRUCT(tuple);
- 
  	if (classtuple->relhasindex != hasindex)
  	{
  		classtuple->relhasindex = hasindex;
--- 1004,1016 ----
  
  	if (!HeapTupleIsValid(tuple))
  		elog(ERROR, "could not find tuple for relation %u", relid);
+ 	classtuple = (Form_pg_class) GETSTRUCT(tuple);
+ 
+ 	/* Apply required updates */
  
  	if (pg_class_scan)
  		LockBuffer(pg_class_scan->rs_cbuf, BUFFER_LOCK_EXCLUSIVE);
  
  	if (classtuple->relhasindex != hasindex)
  	{
  		classtuple->relhasindex = hasindex;
***************
*** 1140,1219 ****
  	Relation	pg_class;
  	HeapTuple	tuple;
  	Form_pg_class rd_rel;
- 	HeapScanDesc pg_class_scan = NULL;
- 	bool		in_place_upd;
  	RelationData workrel;
  
! 	Assert(!IsSystemRelation(relation) || IsToastRelation(relation) ||
  		   relation->rd_rel->relkind == RELKIND_INDEX);
  
  	/* Allocate a new relfilenode */
  	newrelfilenode = newoid();
  
  	/*
! 	 * Find the RELATION relation tuple for the given relation.
  	 */
  	pg_class = heap_openr(RelationRelationName, RowExclusiveLock);
  
! 	in_place_upd = IsIgnoringSystemIndexes();
! 
! 	if (!in_place_upd)
! 	{
! 		tuple = SearchSysCacheCopy(RELOID,
! 							ObjectIdGetDatum(RelationGetRelid(relation)),
! 								   0, 0, 0);
! 	}
! 	else
! 	{
! 		ScanKeyData key[1];
! 
! 		ScanKeyEntryInitialize(&key[0], 0,
! 							   ObjectIdAttributeNumber,
! 							   F_OIDEQ,
! 						   ObjectIdGetDatum(RelationGetRelid(relation)));
! 
! 		pg_class_scan = heap_beginscan(pg_class, SnapshotNow, 1, key);
! 		tuple = heap_getnext(pg_class_scan, ForwardScanDirection);
! 	}
! 
  	if (!HeapTupleIsValid(tuple))
  		elog(ERROR, "could not find tuple for relation %u",
  			 RelationGetRelid(relation));
  	rd_rel = (Form_pg_class) GETSTRUCT(tuple);
  
- 	/* schedule unlinking old relfilenode */
- 	smgrunlink(DEFAULT_SMGR, relation);
- 
  	/* create another storage file. Is it a little ugly ? */
  	memcpy((char *) &workrel, relation, sizeof(RelationData));
  	workrel.rd_fd = -1;
  	workrel.rd_node.relNode = newrelfilenode;
  	heap_storage_create(&workrel);
  	smgrclose(DEFAULT_SMGR, &workrel);
  
  	/* update the pg_class row */
! 	if (in_place_upd)
! 	{
! 		LockBuffer(pg_class_scan->rs_cbuf, BUFFER_LOCK_EXCLUSIVE);
! 		rd_rel->relfilenode = newrelfilenode;
! 		LockBuffer(pg_class_scan->rs_cbuf, BUFFER_LOCK_UNLOCK);
! 		WriteNoReleaseBuffer(pg_class_scan->rs_cbuf);
! 		BufferSync();
! 		/* Send out shared cache inval if necessary */
! 		if (!IsBootstrapProcessingMode())
! 			CacheInvalidateHeapTuple(pg_class, tuple);
! 	}
! 	else
! 	{
! 		rd_rel->relfilenode = newrelfilenode;
! 		simple_heap_update(pg_class, &tuple->t_self, tuple);
! 		CatalogUpdateIndexes(pg_class, tuple);
! 	}
  
! 	if (!pg_class_scan)
! 		heap_freetuple(tuple);
! 	else
! 		heap_endscan(pg_class_scan);
  
  	heap_close(pg_class, RowExclusiveLock);
  
--- 1079,1126 ----
  	Relation	pg_class;
  	HeapTuple	tuple;
  	Form_pg_class rd_rel;
  	RelationData workrel;
  
! 	/* Can't change relfilenode for nailed tables (indexes ok though) */
! 	Assert(!relation->rd_isnailed ||
  		   relation->rd_rel->relkind == RELKIND_INDEX);
+ 	/* Can't change for shared tables or indexes */
+ 	Assert(!relation->rd_rel->relisshared);
  
  	/* Allocate a new relfilenode */
  	newrelfilenode = newoid();
  
  	/*
! 	 * Find the pg_class tuple for the given relation.  This is not used
! 	 * during bootstrap, so okay to use heap_update always.
  	 */
  	pg_class = heap_openr(RelationRelationName, RowExclusiveLock);
  
! 	tuple = SearchSysCacheCopy(RELOID,
! 							   ObjectIdGetDatum(RelationGetRelid(relation)),
! 							   0, 0, 0);
  	if (!HeapTupleIsValid(tuple))
  		elog(ERROR, "could not find tuple for relation %u",
  			 RelationGetRelid(relation));
  	rd_rel = (Form_pg_class) GETSTRUCT(tuple);
  
  	/* create another storage file. Is it a little ugly ? */
+ 	/* NOTE: any conflict in relfilenode value will be caught here */
  	memcpy((char *) &workrel, relation, sizeof(RelationData));
  	workrel.rd_fd = -1;
  	workrel.rd_node.relNode = newrelfilenode;
  	heap_storage_create(&workrel);
  	smgrclose(DEFAULT_SMGR, &workrel);
  
+ 	/* schedule unlinking old relfilenode */
+ 	smgrunlink(DEFAULT_SMGR, relation);
+ 
  	/* update the pg_class row */
! 	rd_rel->relfilenode = newrelfilenode;
! 	simple_heap_update(pg_class, &tuple->t_self, tuple);
! 	CatalogUpdateIndexes(pg_class, tuple);
  
! 	heap_freetuple(tuple);
  
  	heap_close(pg_class, RowExclusiveLock);
  
***************
*** 1263,1273 ****
  	whichRel = relation_open(relid, ShareLock);
  
  	/*
! 	 * Find the RELATION relation tuple for the given relation.
  	 */
  	pg_class = heap_openr(RelationRelationName, RowExclusiveLock);
  
! 	in_place_upd = (IsIgnoringSystemIndexes() || IsReindexProcessing());
  
  	if (!in_place_upd)
  	{
--- 1170,1190 ----
  	whichRel = relation_open(relid, ShareLock);
  
  	/*
! 	 * Find the tuple to update in pg_class.  Normally we make a copy of
! 	 * the tuple using the syscache, modify it, and apply heap_update.
! 	 * But in bootstrap mode we can't use heap_update, so we cheat and
! 	 * overwrite the tuple in-place.
! 	 *
! 	 * We also must cheat if reindexing pg_class itself, because the
! 	 * target index may presently not be part of the set of indexes that
! 	 * CatalogUpdateIndexes would update (see reindex_relation).  In this
! 	 * case the stats updates will not be WAL-logged and so could be lost
! 	 * in a crash.  This seems OK considering VACUUM does the same thing.
  	 */
  	pg_class = heap_openr(RelationRelationName, RowExclusiveLock);
  
! 	in_place_upd = IsBootstrapProcessingMode() ||
! 		ReindexIsProcessingHeap(RelationGetRelid(pg_class));
  
  	if (!in_place_upd)
  	{
***************
*** 1290,1295 ****
--- 1207,1213 ----
  
  	if (!HeapTupleIsValid(tuple))
  		elog(ERROR, "could not find tuple for relation %u", relid);
+ 	rd_rel = (Form_pg_class) GETSTRUCT(tuple);
  
  	/*
  	 * Figure values to insert.
***************
*** 1330,1347 ****
  	 * also reduces the window wherein concurrent CREATE INDEX commands
  	 * may conflict.)
  	 */
- 	rd_rel = (Form_pg_class) GETSTRUCT(tuple);
- 
  	if (rd_rel->relpages != (int32) relpages ||
  		rd_rel->reltuples != (float4) reltuples)
  	{
  		if (in_place_upd)
  		{
! 			/*
! 			 * At bootstrap time, we don't need to worry about concurrency
! 			 * or visibility of changes, so we cheat.  Also cheat if
! 			 * REINDEX.
! 			 */
  			LockBuffer(pg_class_scan->rs_cbuf, BUFFER_LOCK_EXCLUSIVE);
  			rd_rel->relpages = (int32) relpages;
  			rd_rel->reltuples = (float4) reltuples;
--- 1248,1259 ----
  	 * also reduces the window wherein concurrent CREATE INDEX commands
  	 * may conflict.)
  	 */
  	if (rd_rel->relpages != (int32) relpages ||
  		rd_rel->reltuples != (float4) reltuples)
  	{
  		if (in_place_upd)
  		{
! 			/* Bootstrap or reindex case: overwrite fields in place. */
  			LockBuffer(pg_class_scan->rs_cbuf, BUFFER_LOCK_EXCLUSIVE);
  			rd_rel->relpages = (int32) relpages;
  			rd_rel->reltuples = (float4) reltuples;
***************
*** 1561,1570 ****
  					 * should not see any tuples inserted by open
  					 * transactions --- unless it's our own transaction.
  					 * (Consider INSERT followed by CREATE INDEX within a
! 					 * transaction.)
  					 */
  					if (!TransactionIdIsCurrentTransactionId(
! 							  HeapTupleHeaderGetXmin(heapTuple->t_data)))
  						elog(ERROR, "concurrent insert in progress");
  					indexIt = true;
  					tupleIsAlive = true;
--- 1473,1485 ----
  					 * should not see any tuples inserted by open
  					 * transactions --- unless it's our own transaction.
  					 * (Consider INSERT followed by CREATE INDEX within a
! 					 * transaction.)  An exception occurs when reindexing
! 					 * a system catalog, because we often release lock on
! 					 * system catalogs before committing.
  					 */
  					if (!TransactionIdIsCurrentTransactionId(
! 							  HeapTupleHeaderGetXmin(heapTuple->t_data))
! 						&& !IsSystemRelation(heapRelation))
  						elog(ERROR, "concurrent insert in progress");
  					indexIt = true;
  					tupleIsAlive = true;
***************
*** 1576,1585 ****
  					 * should not see any tuples deleted by open
  					 * transactions --- unless it's our own transaction.
  					 * (Consider DELETE followed by CREATE INDEX within a
! 					 * transaction.)
  					 */
  					if (!TransactionIdIsCurrentTransactionId(
! 							  HeapTupleHeaderGetXmax(heapTuple->t_data)))
  						elog(ERROR, "concurrent delete in progress");
  					indexIt = true;
  					tupleIsAlive = false;
--- 1491,1503 ----
  					 * should not see any tuples deleted by open
  					 * transactions --- unless it's our own transaction.
  					 * (Consider DELETE followed by CREATE INDEX within a
! 					 * transaction.)  An exception occurs when reindexing
! 					 * a system catalog, because we often release lock on
! 					 * system catalogs before committing.
  					 */
  					if (!TransactionIdIsCurrentTransactionId(
! 							  HeapTupleHeaderGetXmax(heapTuple->t_data))
! 						&& !IsSystemRelation(heapRelation))
  						elog(ERROR, "concurrent delete in progress");
  					indexIt = true;
  					tupleIsAlive = false;
***************
*** 1692,1754 ****
  /*
   * reindex_index - This routine is used to recreate an index
   */
! bool
! reindex_index(Oid indexId, bool force, bool inplace)
  {
  	Relation	iRel,
  				heapRelation;
  	IndexInfo  *indexInfo;
  	Oid			heapId;
! 	bool		old;
  
  	/*
  	 * Open our index relation and get an exclusive lock on it.
  	 *
! 	 * Note: doing this before opening the parent heap relation means there's
! 	 * a possibility for deadlock failure against another xact that is
! 	 * doing normal accesses to the heap and index.  However, it's not
! 	 * real clear why you'd be needing to do REINDEX on a table that's in
! 	 * active use, so I'd rather have the protection of making sure the
! 	 * index is locked down.
  	 */
  	iRel = index_open(indexId);
  	LockRelation(iRel, AccessExclusiveLock);
  
- 	old = SetReindexProcessing(true);
- 
  	/* Get OID of index's parent table */
  	heapId = iRel->rd_index->indrelid;
  
! 	/* Open the parent heap relation */
  	heapRelation = heap_open(heapId, AccessExclusiveLock);
  
  	/*
  	 * If it's a shared index, we must do inplace processing (because we
! 	 * have no way to update relfilenode in other databases).  Also, if
! 	 * it's a nailed-in-cache index, we must do inplace processing because
! 	 * the relcache can't cope with changing its relfilenode.
  	 *
! 	 * In either of these cases, we are definitely processing a system index,
! 	 * so we'd better be ignoring system indexes.  (These checks are just
! 	 * for paranoia's sake --- upstream code should have disallowed reindex
! 	 * in such cases already.)
! 	 */
! 	if (iRel->rd_rel->relisshared)
! 	{
! 		if (!IsIgnoringSystemIndexes())
! 			elog(ERROR,
! 				 "must be ignoring system indexes to reindex shared index %u",
! 				 indexId);
! 		inplace = true;
! 	}
! 	if (iRel->rd_isnailed)
! 	{
! 		if (!IsIgnoringSystemIndexes())
! 			elog(ERROR,
! 				 "must be ignoring system indexes to reindex nailed index %u",
! 				 indexId);
! 		inplace = true;
! 	}
  
  	/* Fetch info needed for index_build */
  	indexInfo = BuildIndexInfo(iRel);
--- 1610,1663 ----
  /*
   * reindex_index - This routine is used to recreate an index
   */
! void
! reindex_index(Oid indexId)
  {
  	Relation	iRel,
  				heapRelation;
  	IndexInfo  *indexInfo;
  	Oid			heapId;
! 	bool		inplace;
  
  	/*
  	 * Open our index relation and get an exclusive lock on it.
  	 *
! 	 * Note: for REINDEX INDEX, doing this before opening the parent heap
! 	 * relation means there's a possibility for deadlock failure against
! 	 * another xact that is doing normal accesses to the heap and index.
! 	 * However, it's not real clear why you'd be wanting to do REINDEX INDEX
! 	 * on a table that's in active use, so I'd rather have the protection of
! 	 * making sure the index is locked down.  In the REINDEX TABLE and
! 	 * REINDEX DATABASE cases, there is no problem because caller already
! 	 * holds exclusive lock on the parent table.
  	 */
  	iRel = index_open(indexId);
  	LockRelation(iRel, AccessExclusiveLock);
  
  	/* Get OID of index's parent table */
  	heapId = iRel->rd_index->indrelid;
  
! 	/* Open and lock the parent heap relation */
  	heapRelation = heap_open(heapId, AccessExclusiveLock);
  
+ 	SetReindexProcessing(heapId, indexId);
+ 
  	/*
  	 * If it's a shared index, we must do inplace processing (because we
! 	 * have no way to update relfilenode in other databases).  Otherwise
! 	 * we can do it the normal transaction-safe way.
  	 *
! 	 * Since inplace processing isn't crash-safe, we only allow it in a
! 	 * standalone backend.  (In the REINDEX TABLE and REINDEX DATABASE cases,
! 	 * the caller should have detected this.)
! 	 */
! 	inplace = iRel->rd_rel->relisshared;
! 
! 	if (inplace && IsUnderPostmaster)
! 		ereport(ERROR,
! 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! 				 errmsg("shared index \"%s\" can only be reindexed in standalone mode",
! 						RelationGetRelationName(iRel))));
  
  	/* Fetch info needed for index_build */
  	indexInfo = BuildIndexInfo(iRel);
***************
*** 1781,1902 ****
  	 * index_build will close both the heap and index relations (but not
  	 * give up the locks we hold on them).	So we're done.
  	 */
! 
! 	SetReindexProcessing(old);
! 
! 	return true;
! }
! 
! #ifdef NOT_USED
! /*
!  * activate_indexes_of_a_table
!  *	activate/deactivate indexes of the specified table.
!  *
!  * Caller must already hold exclusive lock on the table.
!  */
! bool
! activate_indexes_of_a_table(Relation heaprel, bool activate)
! {
! 	if (IndexesAreActive(heaprel))
! 	{
! 		if (!activate)
! 			setRelhasindex(RelationGetRelid(heaprel), false, false,
! 						   InvalidOid);
! 		else
! 			return false;
! 	}
! 	else
! 	{
! 		if (activate)
! 			reindex_relation(RelationGetRelid(heaprel), false);
! 		else
! 			return false;
! 	}
! 	return true;
  }
- #endif   /* NOT_USED */
  
  /*
   * reindex_relation - This routine is used to recreate all indexes
   * of a relation.
   */
  bool
! reindex_relation(Oid relid, bool force)
  {
- 	Relation	indexRelation;
- 	ScanKeyData entry;
- 	HeapScanDesc scan;
- 	HeapTuple	indexTuple;
- 	bool		old,
- 				reindexed;
- 	bool		overwrite;
  	Relation	rel;
! 
! 	overwrite = false;
  
  	/*
  	 * Ensure to hold an exclusive lock throughout the transaction. The
! 	 * lock could be less intensive (in the non-overwrite path) but for
! 	 * now it's AccessExclusiveLock for simplicity.
  	 */
  	rel = heap_open(relid, AccessExclusiveLock);
  
  	/*
! 	 * Should be ignoring system indexes if we are reindexing a system table.
! 	 * (This is elog not ereport because caller should have caught it.)
! 	 */
! 	if (!IsIgnoringSystemIndexes() &&
! 		IsSystemRelation(rel) && !IsToastRelation(rel))
! 		elog(ERROR,
! 			 "must be ignoring system indexes to reindex system table %u",
! 			 relid);
  
  	/*
! 	 * Shared system indexes must be overwritten because it's impossible
! 	 * to update pg_class tuples of all databases.
  	 */
! 	if (rel->rd_rel->relisshared)
  	{
! 		if (!IsIgnoringSystemIndexes())		/* shouldn't happen */
! 			elog(ERROR,
! 				 "must be ignoring system indexes to reindex shared table %u",
! 				 relid);
! 		overwrite = true;
! 	}
  
! 	old = SetReindexProcessing(true);
  
! 	/*
! 	 * Continue to hold the lock.
! 	 */
! 	heap_close(rel, NoLock);
  
! 	/*
! 	 * Find table's indexes by looking in pg_index (not trusting indexes...)
! 	 */
! 	indexRelation = heap_openr(IndexRelationName, AccessShareLock);
! 	ScanKeyEntryInitialize(&entry, 0,
! 						   Anum_pg_index_indrelid,
! 						   F_OIDEQ,
! 						   ObjectIdGetDatum(relid));
! 	scan = heap_beginscan(indexRelation, SnapshotNow, 1, &entry);
! 	reindexed = false;
! 	while ((indexTuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
! 	{
! 		Form_pg_index index = (Form_pg_index) GETSTRUCT(indexTuple);
  
! 		if (reindex_index(index->indexrelid, false, overwrite))
! 			reindexed = true;
! 		else
! 		{
! 			reindexed = false;
! 			break;
! 		}
  	}
- 	heap_endscan(scan);
- 	heap_close(indexRelation, AccessShareLock);
  
! 	SetReindexProcessing(old);
  
! 	return reindexed;
  }
--- 1690,1770 ----
  	 * index_build will close both the heap and index relations (but not
  	 * give up the locks we hold on them).	So we're done.
  	 */
! 	SetReindexProcessing(InvalidOid, InvalidOid);
  }
  
  /*
   * reindex_relation - This routine is used to recreate all indexes
   * of a relation.
+  *
+  * Returns true if any indexes were rebuilt.
   */
  bool
! reindex_relation(Oid relid)
  {
  	Relation	rel;
! 	bool		is_pg_class;
! 	List	   *indexIds,
! 			   *doneIndexes,
! 			   *indexId;
  
  	/*
  	 * Ensure to hold an exclusive lock throughout the transaction. The
! 	 * lock could perhaps be less intensive (in the non-overwrite case)
! 	 * but for now it's AccessExclusiveLock for simplicity.
  	 */
  	rel = heap_open(relid, AccessExclusiveLock);
  
  	/*
! 	 * Get the list of index OIDs for this relation.  (We trust to the
! 	 * relcache to get this with a sequential scan if ignoring system
! 	 * indexes.)
! 	 */
! 	indexIds = RelationGetIndexList(rel);
  
  	/*
! 	 * reindex_index will attempt to update the pg_class rows for the
! 	 * relation and index.  If we are processing pg_class itself, we
! 	 * want to make sure that the updates do not try to insert index
! 	 * entries into indexes we have not processed yet.  (When we are
! 	 * trying to recover from corrupted indexes, that could easily
! 	 * cause a crash.)  We can accomplish this because CatalogUpdateIndexes
! 	 * will use the relcache's index list to know which indexes to update.
! 	 * We just force the index list to be only the stuff we've processed.
! 	 *
! 	 * It is okay to not insert entries into the indexes we have not
! 	 * processed yet because all of this is transaction-safe.  If we fail
! 	 * partway through, the updated rows are dead and it doesn't matter
! 	 * whether they have index entries.  Also, a new pg_class index will
! 	 * be created with an entry for its own pg_class row because we do
! 	 * setNewRelfilenode() before we do index_build().
  	 */
! 	is_pg_class = (RelationGetRelid(rel) == RelOid_pg_class);
! 	doneIndexes = NIL;
! 
! 	/* Reindex all the indexes. */
! 	foreach(indexId, indexIds)
  	{
! 		Oid		indexOid = lfirsto(indexId);
  
! 		if (is_pg_class)
! 			RelationSetIndexList(rel, doneIndexes);
  
! 		reindex_index(indexOid);
  
! 		CommandCounterIncrement();
  
! 		if (is_pg_class)
! 			doneIndexes = lappendo(doneIndexes, indexOid);
  	}
  
! 	if (is_pg_class)
! 		RelationSetIndexList(rel, indexIds);
! 
! 	/*
! 	 * Close rel, but continue to hold the lock.
! 	 */
! 	heap_close(rel, NoLock);
  
! 	return (indexIds != NIL);
  }
*** src/backend/commands/indexcmds.c.orig	Fri Sep 19 16:07:33 2003
--- src/backend/commands/indexcmds.c	Mon Sep 22 18:22:19 2003
***************
*** 112,125 ****
  	relationId = RelationGetRelid(rel);
  	namespaceId = RelationGetNamespace(rel);
  
- 	if (!IsBootstrapProcessingMode() &&
- 		IsSystemRelation(rel) &&
- 		!IndexesAreActive(rel))
- 		ereport(ERROR,
- 				(errcode(ERRCODE_INDEXES_DEACTIVATED),
- 				 errmsg("existing indexes are inactive"),
- 				 errhint("REINDEX the table first.")));
- 
  	heap_close(rel, NoLock);
  
  	/*
--- 112,117 ----
***************
*** 599,608 ****
  {
  	Oid			indOid;
  	HeapTuple	tuple;
- 	bool		overwrite;
- 
- 	/* Choose in-place-or-not mode */
- 	overwrite = IsIgnoringSystemIndexes();
  
  	indOid = RangeVarGetRelid(indexRelation, false);
  	tuple = SearchSysCache(RELOID,
--- 591,596 ----
***************
*** 617,653 ****
  				 errmsg("relation \"%s\" is not an index",
  						indexRelation->relname)));
  
! 	if (IsSystemClass((Form_pg_class) GETSTRUCT(tuple)) &&
! 		!IsToastClass((Form_pg_class) GETSTRUCT(tuple)))
! 	{
! 		if (!allowSystemTableMods)
! 			ereport(ERROR,
! 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! 					 errmsg("permission denied: \"%s\" is a system index",
! 							indexRelation->relname),
! 					 errhint("Do REINDEX in standalone postgres with -O -P options.")));
! 		if (!IsIgnoringSystemIndexes())
! 			ereport(ERROR,
! 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! 					 errmsg("permission denied: \"%s\" is a system index",
! 							indexRelation->relname),
! 					 errhint("Do REINDEX in standalone postgres with -P -O options.")));
! 	}
  
  	ReleaseSysCache(tuple);
  
! 	/*
! 	 * In-place REINDEX within a transaction block is dangerous, because
! 	 * if the transaction is later rolled back we have no way to undo
! 	 * truncation of the index's physical file.  Disallow it.
! 	 */
! 	if (overwrite)
! 		PreventTransactionChain((void *) indexRelation, "REINDEX");
! 
! 	if (!reindex_index(indOid, force, overwrite))
! 		ereport(WARNING,
! 				(errmsg("index \"%s\" wasn't reindexed",
! 						indexRelation->relname)));
  }
  
  /*
--- 605,618 ----
  				 errmsg("relation \"%s\" is not an index",
  						indexRelation->relname)));
  
! 	/* Check permissions */
! 	if (!pg_class_ownercheck(indOid, GetUserId()))
! 		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
! 					   indexRelation->relname);
  
  	ReleaseSysCache(tuple);
  
! 	reindex_index(indOid);
  }
  
  /*
***************
*** 655,661 ****
   *		Recreate indexes of a table.
   */
  void
! ReindexTable(RangeVar *relation, bool force)
  {
  	Oid			heapOid;
  	HeapTuple	tuple;
--- 620,626 ----
   *		Recreate indexes of a table.
   */
  void
! ReindexTable(RangeVar *relation, bool force /* currently unused */ )
  {
  	Oid			heapOid;
  	HeapTuple	tuple;
***************
*** 674,712 ****
  				 errmsg("relation \"%s\" is not a table",
  						relation->relname)));
  
! 	if (IsSystemClass((Form_pg_class) GETSTRUCT(tuple)) &&
! 		!IsToastClass((Form_pg_class) GETSTRUCT(tuple)))
! 	{
! 		if (!allowSystemTableMods)
! 			ereport(ERROR,
! 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! 					 errmsg("permission denied: \"%s\" is a system table",
! 							relation->relname),
! 					 errhint("Do REINDEX in standalone postgres with -O -P options.")));
! 		if (!IsIgnoringSystemIndexes())
! 			ereport(ERROR,
! 					(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! 					 errmsg("permission denied: \"%s\" is a system table",
! 							relation->relname),
! 					 errhint("Do REINDEX in standalone postgres with -P -O options.")));
! 	}
  
! 	ReleaseSysCache(tuple);
  
! 	/*
! 	 * In-place REINDEX within a transaction block is dangerous, because
! 	 * if the transaction is later rolled back we have no way to undo
! 	 * truncation of the index's physical file.  Disallow it.
! 	 *
! 	 * XXX we assume that in-place reindex will only be done if
! 	 * IsIgnoringSystemIndexes() is true.
! 	 */
! 	if (IsIgnoringSystemIndexes())
! 		PreventTransactionChain((void *) relation, "REINDEX");
  
! 	if (!reindex_relation(heapOid, force))
  		ereport(WARNING,
! 				(errmsg("table \"%s\" wasn't reindexed",
  						relation->relname)));
  }
  
--- 639,661 ----
  				 errmsg("relation \"%s\" is not a table",
  						relation->relname)));
  
! 	/* Check permissions */
! 	if (!pg_class_ownercheck(heapOid, GetUserId()))
! 		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_CLASS,
! 					   relation->relname);
  
! 	/* Can't reindex shared tables except in standalone mode */
! 	if (((Form_pg_class) GETSTRUCT(tuple))->relisshared && IsUnderPostmaster)
! 		ereport(ERROR,
! 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
! 				 errmsg("shared table \"%s\" can only be reindexed in standalone mode",
! 						relation->relname)));
  
! 	ReleaseSysCache(tuple);
  
! 	if (!reindex_relation(heapOid))
  		ereport(WARNING,
! 				(errmsg("table \"%s\" has no indexes",
  						relation->relname)));
  }
  
***************
*** 715,732 ****
   *		Recreate indexes of a database.
   */
  void
! ReindexDatabase(const char *dbname, bool force, bool all)
  {
  	Relation	relationRelation;
  	HeapScanDesc scan;
  	HeapTuple	tuple;
  	MemoryContext private_context;
  	MemoryContext old;
! 	int			relcnt,
! 				relalc,
! 				i,
! 				oncealc = 200;
! 	Oid		   *relids = (Oid *) NULL;
  
  	AssertArg(dbname);
  
--- 664,678 ----
   *		Recreate indexes of a database.
   */
  void
! ReindexDatabase(const char *dbname, bool force /* currently unused */,
! 				bool all)
  {
  	Relation	relationRelation;
  	HeapScanDesc scan;
  	HeapTuple	tuple;
  	MemoryContext private_context;
  	MemoryContext old;
! 	List	   *relids = NIL;
  
  	AssertArg(dbname);
  
***************
*** 739,759 ****
  		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
  					   dbname);
  
- 	if (!allowSystemTableMods)
- 		ereport(ERROR,
- 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- 				 errmsg("REINDEX DATABASE must be done in standalone postgres with -O -P options")));
- 	if (!IsIgnoringSystemIndexes())
- 		ereport(ERROR,
- 				(errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
- 				 errmsg("REINDEX DATABASE must be done in standalone postgres with -P -O options")));
- 
  	/*
  	 * We cannot run inside a user transaction block; if we were inside a
  	 * transaction, then our commit- and start-transaction-command calls
  	 * would not have the intended effect!
  	 */
! 	PreventTransactionChain((void *) dbname, "REINDEX");
  
  	/*
  	 * Create a memory context that will survive forced transaction
--- 685,696 ----
  		aclcheck_error(ACLCHECK_NOT_OWNER, ACL_KIND_DATABASE,
  					   dbname);
  
  	/*
  	 * We cannot run inside a user transaction block; if we were inside a
  	 * transaction, then our commit- and start-transaction-command calls
  	 * would not have the intended effect!
  	 */
! 	PreventTransactionChain((void *) dbname, "REINDEX DATABASE");
  
  	/*
  	 * Create a memory context that will survive forced transaction
***************
*** 768,821 ****
  											ALLOCSET_DEFAULT_MAXSIZE);
  
  	/*
  	 * Scan pg_class to build a list of the relations we need to reindex.
  	 */
  	relationRelation = heap_openr(RelationRelationName, AccessShareLock);
  	scan = heap_beginscan(relationRelation, SnapshotNow, 0, NULL);
- 	relcnt = relalc = 0;
  	while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
  	{
! 		char		relkind;
  
! 		if (!all)
  		{
! 			if (!(IsSystemClass((Form_pg_class) GETSTRUCT(tuple)) &&
! 				  !IsToastClass((Form_pg_class) GETSTRUCT(tuple))))
  				continue;
  		}
! 		relkind = ((Form_pg_class) GETSTRUCT(tuple))->relkind;
! 		if (relkind == RELKIND_RELATION || relkind == RELKIND_TOASTVALUE)
  		{
! 			old = MemoryContextSwitchTo(private_context);
! 			if (relcnt == 0)
! 			{
! 				relalc = oncealc;
! 				relids = palloc(sizeof(Oid) * relalc);
! 			}
! 			else if (relcnt >= relalc)
! 			{
! 				relalc *= 2;
! 				relids = repalloc(relids, sizeof(Oid) * relalc);
! 			}
! 			MemoryContextSwitchTo(old);
! 			relids[relcnt] = HeapTupleGetOid(tuple);
! 			relcnt++;
  		}
  	}
  	heap_endscan(scan);
  	heap_close(relationRelation, AccessShareLock);
  
  	/* Now reindex each rel in a separate transaction */
  	CommitTransactionCommand();
! 	for (i = 0; i < relcnt; i++)
  	{
  		StartTransactionCommand();
  		SetQuerySnapshot();		/* might be needed for functions in
  								 * indexes */
! 		if (reindex_relation(relids[i], force))
  			ereport(NOTICE,
! 					(errmsg("relation %u was reindexed", relids[i])));
  		CommitTransactionCommand();
  	}
  	StartTransactionCommand();
  
--- 705,769 ----
  											ALLOCSET_DEFAULT_MAXSIZE);
  
  	/*
+ 	 * We always want to reindex pg_class first.  This ensures that if
+ 	 * there is any corruption in pg_class' indexes, they will be fixed
+ 	 * before we process any other tables.  This is critical because
+ 	 * reindexing itself will try to update pg_class.
+ 	 */
+ 	old = MemoryContextSwitchTo(private_context);
+ 	relids = lappendo(relids, RelOid_pg_class);
+ 	MemoryContextSwitchTo(old);
+ 
+ 	/*
  	 * Scan pg_class to build a list of the relations we need to reindex.
  	 */
  	relationRelation = heap_openr(RelationRelationName, AccessShareLock);
  	scan = heap_beginscan(relationRelation, SnapshotNow, 0, NULL);
  	while ((tuple = heap_getnext(scan, ForwardScanDirection)) != NULL)
  	{
! 		Form_pg_class classtuple = (Form_pg_class) GETSTRUCT(tuple);
  
! 		if (classtuple->relkind != RELKIND_RELATION &&
! 			classtuple->relkind != RELKIND_TOASTVALUE)
! 			continue;
! 
! 		if (!all)				/* only system tables? */
  		{
! 			if (!(IsSystemClass(classtuple) && !IsToastClass(classtuple)))
  				continue;
  		}
! 
! 		if (IsUnderPostmaster)	/* silently ignore shared tables */
  		{
! 			if (classtuple->relisshared)
! 				continue;
  		}
+ 
+ 		if (HeapTupleGetOid(tuple) == RelOid_pg_class)
+ 			continue;			/* got it already */
+ 
+ 		old = MemoryContextSwitchTo(private_context);
+ 		relids = lappendo(relids, HeapTupleGetOid(tuple));
+ 		MemoryContextSwitchTo(old);
  	}
  	heap_endscan(scan);
  	heap_close(relationRelation, AccessShareLock);
  
  	/* Now reindex each rel in a separate transaction */
  	CommitTransactionCommand();
! 	while (relids)
  	{
+ 		Oid		relid = lfirsto(relids);
+ 
  		StartTransactionCommand();
  		SetQuerySnapshot();		/* might be needed for functions in
  								 * indexes */
! 		if (reindex_relation(relid))
  			ereport(NOTICE,
! 					(errmsg("table \"%s\" was reindexed",
! 							get_rel_name(relid))));
  		CommitTransactionCommand();
+ 		relids = lnext(relids);
  	}
  	StartTransactionCommand();
  
*** src/backend/commands/vacuum.c.orig	Sun Aug  3 23:00:32 2003
--- src/backend/commands/vacuum.c	Sun Sep 21 16:55:42 2003
***************
*** 904,914 ****
  	int			nindexes,
  				i;
  	VRelStats  *vacrelstats;
- 	bool		reindex = false;
- 
- 	if (IsIgnoringSystemIndexes() &&
- 		IsSystemRelation(onerel))
- 		reindex = true;
  
  	vacuum_set_xid_limits(vacstmt, onerel->rd_rel->relisshared,
  						  &OldestXmin, &FreezeLimit);
--- 904,909 ----
***************
*** 927,953 ****
  
  	/* Now open all indexes of the relation */
  	vac_open_indexes(onerel, &nindexes, &Irel);
- 	if (!Irel)
- 		reindex = false;
- 	else if (!RelationGetForm(onerel)->relhasindex)
- 		reindex = true;
  	if (nindexes > 0)
  		vacrelstats->hasindex = true;
  
- #ifdef NOT_USED
- 
- 	/*
- 	 * reindex in VACUUM is dangerous under WAL. ifdef out until it
- 	 * becomes safe.
- 	 */
- 	if (reindex)
- 	{
- 		vac_close_indexes(nindexes, Irel);
- 		Irel = (Relation *) NULL;
- 		activate_indexes_of_a_table(onerel, false);
- 	}
- #endif   /* NOT_USED */
- 
  	/* Clean/scan index relation(s) */
  	if (Irel != (Relation *) NULL)
  	{
--- 922,930 ----
***************
*** 993,1003 ****
  				elog(ERROR, "FlushRelationBuffers returned %d", i);
  		}
  	}
- 
- #ifdef NOT_USED
- 	if (reindex)
- 		activate_indexes_of_a_table(onerel, true);
- #endif   /* NOT_USED */
  
  	/* update shared free space map with final free space info */
  	vac_update_fsm(onerel, &fraged_pages, vacrelstats->rel_pages);
--- 970,975 ----
*** src/backend/executor/execUtils.c.orig	Fri Aug  8 17:47:05 2003
--- src/backend/executor/execUtils.c	Sun Sep 21 16:55:36 2003
***************
*** 647,657 ****
  
  	resultRelInfo->ri_NumIndices = 0;
  
! 	/* checks for disabled indexes */
  	if (!RelationGetForm(resultRelation)->relhasindex)
- 		return;
- 	if (IsIgnoringSystemIndexes() &&
- 		IsSystemRelation(resultRelation))
  		return;
  
  	/*
--- 647,654 ----
  
  	resultRelInfo->ri_NumIndices = 0;
  
! 	/* fast path if no indexes */
  	if (!RelationGetForm(resultRelation)->relhasindex)
  		return;
  
  	/*
*** src/backend/executor/nodeIndexscan.c.orig	Fri Aug 22 16:26:43 2003
--- src/backend/executor/nodeIndexscan.c	Mon Sep 22 18:39:30 2003
***************
*** 964,975 ****
  
  	currentRelation = heap_open(reloid, AccessShareLock);
  
- 	if (!RelationGetForm(currentRelation)->relhasindex)
- 		ereport(ERROR,
- 				(errcode(ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE),
- 				 errmsg("indexes of relation %u were deactivated",
- 						reloid)));
- 
  	indexstate->ss.ss_currentRelation = currentRelation;
  	indexstate->ss.ss_currentScanDesc = NULL;	/* no heap scan here */
  
--- 964,969 ----
*** src/backend/storage/ipc/sinval.c.orig	Tue Aug 19 11:39:55 2003
--- src/backend/storage/ipc/sinval.c	Sun Sep 21 18:54:08 2003
***************
*** 73,78 ****
--- 73,84 ----
  /*
   * ReceiveSharedInvalidMessages
   *		Process shared-cache-invalidation messages waiting for this backend
+  *
+  * NOTE: it is entirely possible for this routine to be invoked recursively
+  * as a consequence of processing inside the invalFunction or resetFunction.
+  * Hence, we must be holding no SI resources when we call them.  The only
+  * bad side-effect is that SIDelExpiredDataEntries might be called extra
+  * times on the way out of a nested call.
   */
  void
  ReceiveSharedInvalidMessages(
*** src/backend/tcop/postgres.c.orig	Sat Sep 13 20:03:32 2003
--- src/backend/tcop/postgres.c	Mon Sep 22 19:30:50 2003
***************
*** 2252,2260 ****
  
  				/*
  				 * ignore system indexes
  				 */
! 				if (secure)		/* XXX safe to allow from client??? */
! 					IgnoreSystemIndexes(true);
  				break;
  
  			case 'o':
--- 2252,2263 ----
  
  				/*
  				 * ignore system indexes
+ 				 *
+ 				 * As of PG 7.4 this is safe to allow from the client,
+ 				 * since it only disables reading the system indexes,
+ 				 * not writing them.  Worst case consequence is slowness.
  				 */
! 				IgnoreSystemIndexes(true);
  				break;
  
  			case 'o':
*** src/backend/tcop/utility.c.orig	Wed Sep 10 11:16:08 2003
--- src/backend/tcop/utility.c	Mon Sep 22 18:16:18 2003
***************
*** 992,1002 ****
  				switch (stmt->kind)
  				{
  					case OBJECT_INDEX:
- 						CheckRelationOwnership(stmt->relation, false);
  						ReindexIndex(stmt->relation, stmt->force);
  						break;
  					case OBJECT_TABLE:
- 						CheckRelationOwnership(stmt->relation, false);
  						ReindexTable(stmt->relation, stmt->force);
  						break;
  					case OBJECT_DATABASE:
--- 992,1000 ----
*** src/backend/utils/cache/relcache.c.orig	Sun Aug  3 23:01:06 2003
--- src/backend/utils/cache/relcache.c	Mon Sep 22 14:32:08 2003
***************
*** 279,287 ****
  
  static void RelationClearRelation(Relation relation, bool rebuild);
  
- #ifdef	ENABLE_REINDEX_NAILED_RELATIONS
  static void RelationReloadClassinfo(Relation relation);
- #endif   /* ENABLE_REINDEX_NAILED_RELATIONS */
  static void RelationFlushRelation(Relation relation);
  static Relation RelationSysNameCacheGetRelation(const char *relationName);
  static bool load_relcache_init_file(void);
--- 279,285 ----
***************
*** 290,296 ****
  static void formrdesc(const char *relationName, int natts,
  		  FormData_pg_attribute *att);
  
! static HeapTuple ScanPgRelation(RelationBuildDescInfo buildinfo);
  static Relation AllocateRelationDesc(Relation relation, Form_pg_class relp);
  static void RelationBuildTupleDesc(RelationBuildDescInfo buildinfo,
  					   Relation relation);
--- 288,294 ----
  static void formrdesc(const char *relationName, int natts,
  		  FormData_pg_attribute *att);
  
! static HeapTuple ScanPgRelation(RelationBuildDescInfo buildinfo, bool indexOK);
  static Relation AllocateRelationDesc(Relation relation, Form_pg_class relp);
  static void RelationBuildTupleDesc(RelationBuildDescInfo buildinfo,
  					   Relation relation);
***************
*** 322,328 ****
   *		and must eventually be freed with heap_freetuple.
   */
  static HeapTuple
! ScanPgRelation(RelationBuildDescInfo buildinfo)
  {
  	HeapTuple	pg_class_tuple;
  	Relation	pg_class_desc;
--- 320,326 ----
   *		and must eventually be freed with heap_freetuple.
   */
  static HeapTuple
! ScanPgRelation(RelationBuildDescInfo buildinfo, bool indexOK)
  {
  	HeapTuple	pg_class_tuple;
  	Relation	pg_class_desc;
***************
*** 367,377 ****
  	/*
  	 * Open pg_class and fetch a tuple.  Force heap scan if we haven't yet
  	 * built the critical relcache entries (this includes initdb and
! 	 * startup without a pg_internal.init file).
  	 */
  	pg_class_desc = heap_openr(RelationRelationName, AccessShareLock);
  	pg_class_scan = systable_beginscan(pg_class_desc, indexRelname,
! 									   criticalRelcachesBuilt,
  									   SnapshotNow,
  									   nkeys, key);
  
--- 365,376 ----
  	/*
  	 * Open pg_class and fetch a tuple.  Force heap scan if we haven't yet
  	 * built the critical relcache entries (this includes initdb and
! 	 * startup without a pg_internal.init file).  The caller can also
! 	 * force a heap scan by setting indexOK == false.
  	 */
  	pg_class_desc = heap_openr(RelationRelationName, AccessShareLock);
  	pg_class_scan = systable_beginscan(pg_class_desc, indexRelname,
! 									   indexOK && criticalRelcachesBuilt,
  									   SnapshotNow,
  									   nkeys, key);
  
***************
*** 834,840 ****
  	/*
  	 * find the tuple in pg_class corresponding to the given relation id
  	 */
! 	pg_class_tuple = ScanPgRelation(buildinfo);
  
  	/*
  	 * if no such tuple exists, return NULL
--- 833,839 ----
  	/*
  	 * find the tuple in pg_class corresponding to the given relation id
  	 */
! 	pg_class_tuple = ScanPgRelation(buildinfo, true);
  
  	/*
  	 * if no such tuple exists, return NULL
***************
*** 875,881 ****
  	 * it could be new too, but it's okay to forget that fact if forced to
  	 * flush the entry.)
  	 */
! 	relation->rd_isnailed = false;
  	relation->rd_isnew = false;
  	relation->rd_istemp = isTempNamespace(relation->rd_rel->relnamespace);
  
--- 874,880 ----
  	 * it could be new too, but it's okay to forget that fact if forced to
  	 * flush the entry.)
  	 */
! 	relation->rd_isnailed = 0;
  	relation->rd_isnew = false;
  	relation->rd_istemp = isTempNamespace(relation->rd_rel->relnamespace);
  
***************
*** 1386,1392 ****
  	 * all entries built with this routine are nailed-in-cache; none are
  	 * for new or temp relations.
  	 */
! 	relation->rd_isnailed = true;
  	relation->rd_isnew = false;
  	relation->rd_istemp = false;
  
--- 1385,1391 ----
  	 * all entries built with this routine are nailed-in-cache; none are
  	 * for new or temp relations.
  	 */
! 	relation->rd_isnailed = 1;
  	relation->rd_isnew = false;
  	relation->rd_istemp = false;
  
***************
*** 1500,1506 ****
   *		Lookup an existing reldesc by OID.
   *
   *		Only try to get the reldesc by looking in the cache,
!  *		do not go to the disk.
   *
   *		NB: relation ref count is incremented if successful.
   *		Caller should eventually decrement count.  (Usually,
--- 1499,1505 ----
   *		Lookup an existing reldesc by OID.
   *
   *		Only try to get the reldesc by looking in the cache,
!  *		do not go to the disk if it's not present.
   *
   *		NB: relation ref count is incremented if successful.
   *		Caller should eventually decrement count.  (Usually,
***************
*** 1514,1520 ****
--- 1513,1524 ----
  	RelationIdCacheLookup(relationId, rd);
  
  	if (RelationIsValid(rd))
+ 	{
  		RelationIncrementReferenceCount(rd);
+ 		/* revalidate nailed index if necessary */
+ 		if (rd->rd_isnailed == 2)
+ 			RelationReloadClassinfo(rd);
+ 	}
  
  	return rd;
  }
***************
*** 1538,1548 ****
--- 1542,1568 ----
  	RelationSysNameCacheLookup(NameStr(name), rd);
  
  	if (RelationIsValid(rd))
+ 	{
  		RelationIncrementReferenceCount(rd);
+ 		/* revalidate nailed index if necessary */
+ 		if (rd->rd_isnailed == 2)
+ 			RelationReloadClassinfo(rd);
+ 	}
  
  	return rd;
  }
  
+ /*
+  *		RelationNodeCacheGetRelation
+  *
+  *		As above, but lookup by relfilenode.
+  *
+  * NOTE: this must NOT try to revalidate invalidated nailed indexes, since
+  * that could cause us to return an entry with a different relfilenode than
+  * the caller asked for.  Currently this is used only by the buffer manager.
+  * Really the bufmgr's idea of relations should be separated out from the
+  * relcache ...
+  */
  Relation
  RelationNodeCacheGetRelation(RelFileNode rnode)
  {
***************
*** 1647,1685 ****
  #endif
  }
  
- #ifdef	ENABLE_REINDEX_NAILED_RELATIONS
  /*
!  * RelationReloadClassinfo
   *
!  *	This function is especially for nailed relations.
!  *	relhasindex/relfilenode could be changed even for
!  *	nailed relations.
   */
  static void
  RelationReloadClassinfo(Relation relation)
  {
  	RelationBuildDescInfo buildinfo;
  	HeapTuple	pg_class_tuple;
  	Form_pg_class relp;
  
! 	if (!relation->rd_rel)
! 		return;
  	buildinfo.infotype = INFO_RELID;
  	buildinfo.i.info_id = relation->rd_id;
! 	pg_class_tuple = ScanPgRelation(buildinfo);
  	if (!HeapTupleIsValid(pg_class_tuple))
  		elog(ERROR, "could not find tuple for system relation %u",
  			 relation->rd_id);
- 	RelationCacheDelete(relation);
  	relp = (Form_pg_class) GETSTRUCT(pg_class_tuple);
! 	memcpy((char *) relation->rd_rel, (char *) relp, CLASS_TUPLE_SIZE);
! 	relation->rd_node.relNode = relp->relfilenode;
! 	RelationCacheInsert(relation);
  	heap_freetuple(pg_class_tuple);
! 
! 	return;
  }
- #endif   /* ENABLE_REINDEX_NAILED_RELATIONS */
  
  /*
   * RelationClearRelation
--- 1667,1726 ----
  #endif
  }
  
  /*
!  * RelationReloadClassinfo - reload the pg_class row (only)
   *
!  *	This function is used only for nailed indexes.  Since a REINDEX can
!  *	change the relfilenode value for a nailed index, we have to reread
!  *	the pg_class row anytime we get an SI invalidation on a nailed index
!  *	(without throwing away the whole relcache entry, since we'd be unable
!  *	to rebuild it).
!  *
!  *	We can't necessarily reread the pg_class row right away; we might be
!  *	in a failed transaction when we receive the SI notification.  If so,
!  *	RelationClearRelation just marks the entry as invalid by setting
!  *	rd_isnailed to 2.  This routine is called to fix the entry when it
!  *	is next needed.
   */
  static void
  RelationReloadClassinfo(Relation relation)
  {
  	RelationBuildDescInfo buildinfo;
+ 	bool		indexOK;
  	HeapTuple	pg_class_tuple;
  	Form_pg_class relp;
  
! 	/* Should be called only for invalidated nailed indexes */
! 	Assert(relation->rd_isnailed == 2 &&
! 		   relation->rd_rel->relkind == RELKIND_INDEX);
! 	/* Read the pg_class row */
  	buildinfo.infotype = INFO_RELID;
  	buildinfo.i.info_id = relation->rd_id;
! 	/*
! 	 * Don't try to use an indexscan of pg_class_oid_index to reload the
! 	 * info for pg_class_oid_index ...
! 	 */
! 	indexOK = strcmp(RelationGetRelationName(relation), ClassOidIndex) != 0;
! 	pg_class_tuple = ScanPgRelation(buildinfo, indexOK);
  	if (!HeapTupleIsValid(pg_class_tuple))
  		elog(ERROR, "could not find tuple for system relation %u",
  			 relation->rd_id);
  	relp = (Form_pg_class) GETSTRUCT(pg_class_tuple);
! 	if (relation->rd_node.relNode != relp->relfilenode)
! 	{
! 		/* We have to re-insert the entry into the relcache indexes */
! 		RelationCacheDelete(relation);
! 		memcpy((char *) relation->rd_rel, (char *) relp, CLASS_TUPLE_SIZE);
! 		relation->rd_node.relNode = relp->relfilenode;
! 		RelationCacheInsert(relation);
! 	}
  	heap_freetuple(pg_class_tuple);
! 	/* Must adjust number of blocks after we know the new relfilenode */
! 	relation->rd_targblock = InvalidBlockNumber;
! 	RelationUpdateNumberOfBlocks(relation);
! 	/* Okay, now it's valid again */
! 	relation->rd_isnailed = 1;
  }
  
  /*
   * RelationClearRelation
***************
*** 1712,1726 ****
  	 * Never, never ever blow away a nailed-in system relation, because
  	 * we'd be unable to recover.  However, we must update rd_nblocks and
  	 * reset rd_targblock, in case we got called because of a relation
! 	 * cache flush that was triggered by VACUUM.
  	 */
  	if (relation->rd_isnailed)
  	{
! 		relation->rd_targblock = InvalidBlockNumber;
! 		RelationUpdateNumberOfBlocks(relation);
! #ifdef	ENABLE_REINDEX_NAILED_RELATIONS
! 		RelationReloadClassinfo(relation);
! #endif   /* ENABLE_REINDEX_NAILED_RELATIONS */
  		return;
  	}
  
--- 1753,1779 ----
  	 * Never, never ever blow away a nailed-in system relation, because
  	 * we'd be unable to recover.  However, we must update rd_nblocks and
  	 * reset rd_targblock, in case we got called because of a relation
! 	 * cache flush that was triggered by VACUUM.  If it's a nailed index,
! 	 * then we need to re-read the pg_class row to see if its relfilenode
! 	 * changed.  We can't necessarily do that here, because we might be in
! 	 * a failed transaction.  We assume it's okay to do it if there are open
! 	 * references to the relcache entry (cf notes for AtEOXact_RelationCache).
! 	 * Otherwise just mark the entry as possibly invalid, and it'll be fixed
! 	 * when next opened.
  	 */
  	if (relation->rd_isnailed)
  	{
! 		if (relation->rd_rel->relkind == RELKIND_INDEX)
! 		{
! 			relation->rd_isnailed = 2;	/* needs to be revalidated */
! 			if (relation->rd_refcnt > 1)
! 				RelationReloadClassinfo(relation);
! 		}
! 		else
! 		{
! 			relation->rd_targblock = InvalidBlockNumber;
! 			RelationUpdateNumberOfBlocks(relation);
! 		}
  		return;
  	}
  
***************
*** 1928,1933 ****
--- 1981,1992 ----
   *	 because (a) during the first pass we won't process any more SI messages,
   *	 so hash_seq_search will complete safely; (b) during the second pass we
   *	 only hold onto pointers to nondeletable entries.
+  *
+  *	 The two-phase approach also makes it easy to ensure that we process
+  *	 nailed-in-cache indexes before other nondeletable items, and that we
+  *	 process pg_class_oid_index first of all.  In scenarios where a nailed
+  *	 index has been given a new relfilenode, we have to detect that update
+  *	 before the nailed index is used in reloading any other relcache entry.
   */
  void
  RelationCacheInvalidate(void)
***************
*** 1935,1940 ****
--- 1994,2000 ----
  	HASH_SEQ_STATUS status;
  	RelIdCacheEnt *idhentry;
  	Relation	relation;
+ 	List	   *rebuildFirstList = NIL;
  	List	   *rebuildList = NIL;
  	List	   *l;
  
***************
*** 1954,1968 ****
  		if (RelationHasReferenceCountZero(relation))
  		{
  			/* Delete this entry immediately */
  			RelationClearRelation(relation, false);
  		}
  		else
  		{
! 			/* Add entry to list of stuff to rebuild in second pass */
! 			rebuildList = lcons(relation, rebuildList);
  		}
  	}
  
  	/* Phase 2: rebuild the items found to need rebuild in phase 1 */
  	foreach(l, rebuildList)
  	{
--- 2014,2046 ----
  		if (RelationHasReferenceCountZero(relation))
  		{
  			/* Delete this entry immediately */
+ 			Assert(!relation->rd_isnailed);
  			RelationClearRelation(relation, false);
  		}
  		else
  		{
! 			/*
! 			 * Add this entry to list of stuff to rebuild in second pass.
! 			 * pg_class_oid_index goes on the front of rebuildFirstList,
! 			 * other nailed indexes on the back, and everything else into
! 			 * rebuildList (in no particular order).
! 			 */
! 			if (relation->rd_isnailed &&
! 				relation->rd_rel->relkind == RELKIND_INDEX)
! 			{
! 				if (strcmp(RelationGetRelationName(relation),
! 						   ClassOidIndex) == 0)
! 					rebuildFirstList = lcons(relation, rebuildFirstList);
! 				else
! 					rebuildFirstList = lappend(rebuildFirstList, relation);
! 			}
! 			else
! 				rebuildList = lcons(relation, rebuildList);
  		}
  	}
  
+ 	rebuildList = nconc(rebuildFirstList, rebuildList);
+ 
  	/* Phase 2: rebuild the items found to need rebuild in phase 1 */
  	foreach(l, rebuildList)
  	{
***************
*** 1976,1981 ****
--- 2054,2064 ----
   * AtEOXact_RelationCache
   *
   *	Clean up the relcache at transaction commit or abort.
+  *
+  * Note: this must be called *before* processing invalidation messages.
+  * In the case of abort, we don't want to try to rebuild any invalidated
+  * cache entries (since we can't safely do database accesses).  Therefore
+  * we must reset refcnts before handling pending invalidations.
   */
  void
  AtEOXact_RelationCache(bool commit)
***************
*** 2045,2050 ****
--- 2128,2143 ----
  			/* abort case, just reset it quietly */
  			RelationSetReferenceCount(relation, expected_refcnt);
  		}
+ 
+ 		/*
+ 		 * Flush any temporary index list.
+ 		 */
+ 		if (relation->rd_indexvalid == 2)
+ 		{
+ 			freeList(relation->rd_indexlist);
+ 			relation->rd_indexlist = NIL;
+ 			relation->rd_indexvalid = 0;
+ 		}
  	}
  }
  
***************
*** 2101,2107 ****
  	 * want it kicked out.	e.g. pg_attribute!!!
  	 */
  	if (nailit)
! 		rel->rd_isnailed = true;
  
  	/*
  	 * create a new tuple descriptor from the one passed in.  We do this
--- 2194,2200 ----
  	 * want it kicked out.	e.g. pg_attribute!!!
  	 */
  	if (nailit)
! 		rel->rd_isnailed = 1;
  
  	/*
  	 * create a new tuple descriptor from the one passed in.  We do this
***************
*** 2288,2294 ****
  			buildinfo.infotype = INFO_RELNAME; \
  			buildinfo.i.info_name = (indname); \
  			ird = RelationBuildDesc(buildinfo, NULL); \
! 			ird->rd_isnailed = true; \
  			RelationSetReferenceCount(ird, 1); \
  		} while (0)
  
--- 2381,2387 ----
  			buildinfo.infotype = INFO_RELNAME; \
  			buildinfo.i.info_name = (indname); \
  			ird = RelationBuildDesc(buildinfo, NULL); \
! 			ird->rd_isnailed = 1; \
  			RelationSetReferenceCount(ird, 1); \
  		} while (0)
  
***************
*** 2575,2581 ****
   * The index list is created only if someone requests it.  We scan pg_index
   * to find relevant indexes, and add the list to the relcache entry so that
   * we won't have to compute it again.  Note that shared cache inval of a
!  * relcache entry will delete the old list and set rd_indexfound to false,
   * so that we must recompute the index list on next request.  This handles
   * creation or deletion of an index.
   *
--- 2668,2674 ----
   * The index list is created only if someone requests it.  We scan pg_index
   * to find relevant indexes, and add the list to the relcache entry so that
   * we won't have to compute it again.  Note that shared cache inval of a
!  * relcache entry will delete the old list and set rd_indexvalid to 0,
   * so that we must recompute the index list on next request.  This handles
   * creation or deletion of an index.
   *
***************
*** 2602,2608 ****
  	MemoryContext oldcxt;
  
  	/* Quick exit if we already computed the list. */
! 	if (relation->rd_indexfound)
  		return listCopy(relation->rd_indexlist);
  
  	/*
--- 2695,2701 ----
  	MemoryContext oldcxt;
  
  	/* Quick exit if we already computed the list. */
! 	if (relation->rd_indexvalid != 0)
  		return listCopy(relation->rd_indexlist);
  
  	/*
***************
*** 2638,2644 ****
  	/* Now save a copy of the completed list in the relcache entry. */
  	oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
  	relation->rd_indexlist = listCopy(result);
! 	relation->rd_indexfound = true;
  	MemoryContextSwitchTo(oldcxt);
  
  	return result;
--- 2731,2737 ----
  	/* Now save a copy of the completed list in the relcache entry. */
  	oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
  	relation->rd_indexlist = listCopy(result);
! 	relation->rd_indexvalid = 1;
  	MemoryContextSwitchTo(oldcxt);
  
  	return result;
***************
*** 2677,2682 ****
--- 2770,2804 ----
  }
  
  /*
+  * RelationSetIndexList -- externally force the index list contents
+  *
+  * This is used to temporarily override what we think the set of valid
+  * indexes is.  The forcing will be valid only until transaction commit
+  * or abort.
+  *
+  * This should only be applied to nailed relations, because in a non-nailed
+  * relation the hacked index list could be lost at any time due to SI
+  * messages.  In practice it is only used on pg_class (see REINDEX).
+  *
+  * It is up to the caller to make sure the given list is correctly ordered.
+  */
+ void
+ RelationSetIndexList(Relation relation, List *indexIds)
+ {
+ 	MemoryContext oldcxt;
+ 
+ 	Assert(relation->rd_isnailed == 1);
+ 	/* Copy the list into the cache context (could fail for lack of mem) */
+ 	oldcxt = MemoryContextSwitchTo(CacheMemoryContext);
+ 	indexIds = listCopy(indexIds);
+ 	MemoryContextSwitchTo(oldcxt);
+ 	/* Okay to replace old list */
+ 	freeList(relation->rd_indexlist);
+ 	relation->rd_indexlist = indexIds;
+ 	relation->rd_indexvalid = 2;		/* mark list as forced */
+ }
+ 
+ /*
   * RelationGetIndexExpressions -- get the index expressions for an index
   *
   * We cache the result of transforming pg_index.indexprs into a node tree.
***************
*** 3087,3093 ****
  			RelationSetReferenceCount(rel, 1);
  		else
  			RelationSetReferenceCount(rel, 0);
! 		rel->rd_indexfound = false;
  		rel->rd_indexlist = NIL;
  		MemSet(&rel->pgstat_info, 0, sizeof(rel->pgstat_info));
  
--- 3209,3215 ----
  			RelationSetReferenceCount(rel, 1);
  		else
  			RelationSetReferenceCount(rel, 0);
! 		rel->rd_indexvalid = 0;
  		rel->rd_indexlist = NIL;
  		MemSet(&rel->pgstat_info, 0, sizeof(rel->pgstat_info));
  
*** src/backend/utils/cache/syscache.c.orig	Sun Aug  3 23:01:06 2003
--- src/backend/utils/cache/syscache.c	Sun Sep 21 16:55:31 2003
***************
*** 436,452 ****
  	}}
  };
  
! static CatCache *SysCache[
! 						  lengthof(cacheinfo)];
  static int	SysCacheSize = lengthof(cacheinfo);
  static bool CacheInitialized = false;
- 
- 
- bool
- IsCacheInitialized(void)
- {
- 	return CacheInitialized;
- }
  
  
  /*
--- 436,444 ----
  	}}
  };
  
! static CatCache *SysCache[lengthof(cacheinfo)];
  static int	SysCacheSize = lengthof(cacheinfo);
  static bool CacheInitialized = false;
  
  
  /*
*** src/backend/utils/init/miscinit.c.orig	Mon Aug  4 00:03:10 2003
--- src/backend/utils/init/miscinit.c	Mon Sep 22 17:44:19 2003
***************
*** 51,56 ****
--- 51,61 ----
  
  /* ----------------------------------------------------------------
   *		ignoring system indexes support stuff
+  *
+  * NOTE: "ignoring system indexes" means we do not use the system indexes
+  * for lookups (either in hardwired catalog accesses or in planner-generated
+  * plans).  We do, however, still update the indexes when a catalog
+  * modification is made.
   * ----------------------------------------------------------------
   */
  
***************
*** 61,80 ****
   *		True if ignoring system indexes.
   */
  bool
! IsIgnoringSystemIndexes()
  {
  	return isIgnoringSystemIndexes;
  }
  
  /*
   * IgnoreSystemIndexes
!  *	Set true or false whether PostgreSQL ignores system indexes.
!  *
   */
  void
  IgnoreSystemIndexes(bool mode)
  {
  	isIgnoringSystemIndexes = mode;
  }
  
  /* ----------------------------------------------------------------
--- 66,131 ----
   *		True if ignoring system indexes.
   */
  bool
! IsIgnoringSystemIndexes(void)
  {
  	return isIgnoringSystemIndexes;
  }
  
  /*
   * IgnoreSystemIndexes
!  *		Set true or false whether PostgreSQL ignores system indexes.
   */
  void
  IgnoreSystemIndexes(bool mode)
  {
  	isIgnoringSystemIndexes = mode;
+ }
+ 
+ /* ----------------------------------------------------------------
+  *		system index reindexing support
+  *
+  * When we are busy reindexing a system index, this code provides support
+  * for preventing catalog lookups from using that index.
+  * ----------------------------------------------------------------
+  */
+ 
+ static Oid	currentlyReindexedHeap = InvalidOid;
+ static Oid	currentlyReindexedIndex = InvalidOid;
+ 
+ /*
+  * ReindexIsProcessingHeap
+  *		True if heap specified by OID is currently being reindexed.
+  */
+ bool
+ ReindexIsProcessingHeap(Oid heapOid)
+ {
+ 	return heapOid == currentlyReindexedHeap;
+ }
+ 
+ /*
+  * ReindexIsProcessingIndex
+  *		True if index specified by OID is currently being reindexed.
+  */
+ bool
+ ReindexIsProcessingIndex(Oid indexOid)
+ {
+ 	return indexOid == currentlyReindexedIndex;
+ }
+ 
+ /*
+  * SetReindexProcessing
+  *		Set flag that specified heap/index are being reindexed.
+  *		Pass InvalidOid to indicate that reindexing is not active.
+  */
+ void
+ SetReindexProcessing(Oid heapOid, Oid indexOid)
+ {
+ 	/* Args should be both, or neither, InvalidOid */
+ 	Assert((heapOid == InvalidOid) == (indexOid == InvalidOid));
+ 	/* Reindexing is not re-entrant. */
+ 	Assert(indexOid == InvalidOid || currentlyReindexedIndex == InvalidOid);
+ 	currentlyReindexedHeap = heapOid;
+ 	currentlyReindexedIndex = indexOid;
  }
  
  /* ----------------------------------------------------------------
*** src/include/catalog/index.h.orig	Sun Aug  3 23:01:28 2003
--- src/include/catalog/index.h	Sun Sep 21 19:25:22 2003
***************
*** 51,65 ****
  			   char *nullv);
  
  extern void UpdateStats(Oid relid, double reltuples);
! extern bool IndexesAreActive(Relation heaprel);
  extern void setRelhasindex(Oid relid, bool hasindex,
  			   bool isprimary, Oid reltoastidxid);
  
  extern void setNewRelfilenode(Relation relation);
  
- extern bool SetReindexProcessing(bool processing);
- extern bool IsReindexProcessing(void);
- 
  extern void index_build(Relation heapRelation, Relation indexRelation,
  			IndexInfo *indexInfo);
  
--- 51,62 ----
  			   char *nullv);
  
  extern void UpdateStats(Oid relid, double reltuples);
! 
  extern void setRelhasindex(Oid relid, bool hasindex,
  			   bool isprimary, Oid reltoastidxid);
  
  extern void setNewRelfilenode(Relation relation);
  
  extern void index_build(Relation heapRelation, Relation indexRelation,
  			IndexInfo *indexInfo);
  
***************
*** 69,77 ****
  				   IndexBuildCallback callback,
  				   void *callback_state);
  
! extern bool activate_indexes_of_a_table(Relation heaprel, bool activate);
! 
! extern bool reindex_index(Oid indexId, bool force, bool inplace);
! extern bool reindex_relation(Oid relid, bool force);
  
  #endif   /* INDEX_H */
--- 66,72 ----
  				   IndexBuildCallback callback,
  				   void *callback_state);
  
! extern void reindex_index(Oid indexId);
! extern bool reindex_relation(Oid relid);
  
  #endif   /* INDEX_H */
*** src/include/miscadmin.h.orig	Tue Aug 26 11:38:25 2003
--- src/include/miscadmin.h	Mon Sep 22 17:44:14 2003
***************
*** 296,313 ****
  extern void BaseInit(void);
  
  /* in utils/init/miscinit.c */
  extern void CreateDataDirLockFile(const char *datadir, bool amPostmaster);
  extern void CreateSocketLockFile(const char *socketfile, bool amPostmaster);
  extern void TouchSocketLockFile(void);
  extern void RecordSharedMemoryInLockFile(unsigned long id1,
  							 unsigned long id2);
- 
  extern void ValidatePgVersion(const char *path);
  extern void process_preload_libraries(char *preload_libraries_string);
- 
- /* these externs do not belong here... */
- extern void IgnoreSystemIndexes(bool mode);
- extern bool IsIgnoringSystemIndexes(void);
- extern bool IsCacheInitialized(void);
  
  #endif   /* MISCADMIN_H */
--- 296,312 ----
  extern void BaseInit(void);
  
  /* in utils/init/miscinit.c */
+ extern void IgnoreSystemIndexes(bool mode);
+ extern bool IsIgnoringSystemIndexes(void);
+ extern void SetReindexProcessing(Oid heapOid, Oid indexOid);
+ extern bool ReindexIsProcessingHeap(Oid heapOid);
+ extern bool ReindexIsProcessingIndex(Oid indexOid);
  extern void CreateDataDirLockFile(const char *datadir, bool amPostmaster);
  extern void CreateSocketLockFile(const char *socketfile, bool amPostmaster);
  extern void TouchSocketLockFile(void);
  extern void RecordSharedMemoryInLockFile(unsigned long id1,
  							 unsigned long id2);
  extern void ValidatePgVersion(const char *path);
  extern void process_preload_libraries(char *preload_libraries_string);
  
  #endif   /* MISCADMIN_H */
*** src/include/utils/errcodes.h.orig	Tue Aug 26 17:15:27 2003
--- src/include/utils/errcodes.h	Mon Sep 22 18:40:53 2003
***************
*** 281,287 ****
  /* Class 55 - Object Not In Prerequisite State (class borrowed from DB2) */
  #define ERRCODE_OBJECT_NOT_IN_PREREQUISITE_STATE	MAKE_SQLSTATE('5','5', '0','0','0')
  #define ERRCODE_OBJECT_IN_USE				MAKE_SQLSTATE('5','5', '0','0','6')
- #define ERRCODE_INDEXES_DEACTIVATED			MAKE_SQLSTATE('5','5', 'P','0','1')
  #define ERRCODE_CANT_CHANGE_RUNTIME_PARAM	MAKE_SQLSTATE('5','5', 'P','0','2')
  
  /* Class 57 - Operator Intervention (class borrowed from DB2) */
--- 281,286 ----
*** src/include/utils/rel.h.orig	Sun Aug  3 23:01:45 2003
--- src/include/utils/rel.h	Mon Sep 22 13:34:07 2003
***************
*** 119,126 ****
  	 * it is possible for new-ness to be "forgotten" (eg, after CLUSTER).
  	 */
  	bool		rd_istemp;		/* rel uses the local buffer mgr */
! 	bool		rd_isnailed;	/* rel is nailed in cache */
! 	bool		rd_indexfound;	/* true if rd_indexlist is valid */
  	Form_pg_class rd_rel;		/* RELATION tuple */
  	TupleDesc	rd_att;			/* tuple descriptor */
  	Oid			rd_id;			/* relation's object id */
--- 119,128 ----
  	 * it is possible for new-ness to be "forgotten" (eg, after CLUSTER).
  	 */
  	bool		rd_istemp;		/* rel uses the local buffer mgr */
! 	char		rd_isnailed;	/* rel is nailed in cache: 0 = no, 1 = yes,
! 								 * 2 = yes but possibly invalid */
! 	char		rd_indexvalid;	/* state of rd_indexlist: 0 = not valid,
! 								 * 1 = valid, 2 = temporarily forced */
  	Form_pg_class rd_rel;		/* RELATION tuple */
  	TupleDesc	rd_att;			/* tuple descriptor */
  	Oid			rd_id;			/* relation's object id */
*** src/include/utils/relcache.h.orig	Sun Aug  3 23:01:45 2003
--- src/include/utils/relcache.h	Sun Sep 21 19:25:15 2003
***************
*** 35,40 ****
--- 35,42 ----
  extern List *RelationGetIndexExpressions(Relation relation);
  extern List *RelationGetIndexPredicate(Relation relation);
  
+ extern void RelationSetIndexList(Relation relation, List *indexIds);
+ 
  extern void RelationInitIndexAccessInfo(Relation relation);
  
  /*
Mon, 22 Sep 2003 20:31:29 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Improving REINDEX for system indexes (long) 
I've just put back your previous change, sorry.
As I already mentioned many times it must be the first thing.

Though I don't remenber my code completely yet, I would
reply to some points. 
Unfortunately REINDEX wasn't a eagerly wanted command when 
I implemented it. Though I wanted to introduce a per index
flag, I unwillingly used an existent per table flag(relhasindex)
instead. Because it was impossible to make REINDEX transaction-safe
then, such flag was needed to suppress inconsistency as less
as possible.
I also unwillingly introduced the ReindexProcessing mode(flag)
because I didn't think of other quick solutions. 

IIRC the main reason why I gave up the REINDEX functionality
on nailed relations was the difficulty of reindexing pg_class
and the handling of relcache overflow. I didn't have much time
to test it. In addtion REINDEX wasn't a recognized command
then and I found no one to discuss the situation.

Tom Lane wrote:
> 
> I've been looking at the issues involved in reindexing system tables,
> and I now have what I think is a fairly defensible set of proposals.
> 
> We should whenever possible use the same reindexing technique used by
> CLUSTER: 

REINDEX was the first command which used the pg_class.relfilenode
functionality. The pg_class.relfilenode was essentially my proposal.

> 1. There is a problem for a shared index, because we have no way to
> update pg_class.relfilenode in all the other databases besides ours.
> I see no good way around this problem.

So the current REINDEX disallows on-line reindex on shared relations.
 
> 2. There is a problem for a nailed-in-cache index, because the relcache
> isn't prepared to cope with relfilenode updates for such indexes.
> However, that is fixable.

My code works pretty good with nailed relations except pg_class
#if defined (ENABLE_REINDEX_NAILED_RELATIONS).

> 3. There is a problem for an index on pg_class itself: doing heap_update
> on a pg_class row must make new index entries.  We have to be careful
> that the new row does appear in the updated index, while not making
> entries in old-and-possibly-broken indexes.  This is doable.

Yes.

Sorry I have no time to continue the discussion now.

regards,
Hiroshi Inoue
	http://www.geocities.jp/inocchichichi/psqlodbc/

Tue, 23 Sep 2003 11:44:27 +0900
Hiroshi Inoue <Inoue@tpf.co.jp>
Re: Improving REINDEX for system indexes (long)
Hiroshi Inoue wrote:
> instead. Because it was impossible to make REINDEX transaction-safe
> then, such flag was needed to suppress inconsistency as less
> as possible.

This mean that the actual REINDEX is not transaction-safe ?


Regards
Gaetano Mendola


Tue, 23 Sep 2003 10:15:08 +0200
Gaetano Mendola <mendola@bigfoot.com>
Re: Improving REINDEX for system indexes (long)
> -----Original Message-----
> From: Gaetano Mendola [mailto:mendola@bigfoot.com] 
> 
> Hiroshi Inoue wrote:
> > instead. Because it was impossible to make REINDEX transaction-safe
> > then, such flag was needed to suppress inconsistency as less
> > as possible.
> 
> This mean that the actual REINDEX is not transaction-safe ?

No.
It was not transaction-safe long time ago.

regards,
Hiroshi Inoue


Tue, 23 Sep 2003 19:45:16 +0900
"Hiroshi Inoue" <inoue@tpf.co.jp>
Re: Improving REINDEX for system indexes (long)
Hiroshi Inoue wrote:
Gaetano Mendola [mailto:mendola@bigfoot.com]  wrote:
>>
>>Hiroshi Inoue wrote:
>>
>>>instead. Because it was impossible to make REINDEX transaction-safe
>>>then, such flag was needed to suppress inconsistency as less
>>>as possible.
>>
>>This mean that the actual REINDEX is not transaction-safe ?
> 
> 
> No.
> It was not transaction-safe long time ago.

Anyway I think there is a nasty bug somewhere, see my last posts
about "duplication primary key".


Regards
Gaetano Mendola










Tue, 23 Sep 2003 16:28:27 +0200
Gaetano Mendola <mendola@bigfoot.com>
Re: Improving REINDEX for system indexes (long)

Where are we on this?  Does the code path now make sense, at least?

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

Hiroshi Inoue wrote:
> > -----Original Message-----
> > From: Marc G. Fournier [mailto:scrappy@postgresql.org] 
> > Sent: Sunday, September 21, 2003 6:45 AM
> > To: Hiroshi Inoue
> > Cc: 'Tom Lane'; pgsql-committers@postgresql.org
> > Subject: Re: [COMMITTERS] pgsql-server/src/backend 
> > catalog/index.c comma ...
> > 
> > On Sun, 21 Sep 2003, Hiroshi Inoue wrote:
> > 
> > > > "Hiroshi Inoue" <inoue@tpf.co.jp> writes:
> > > > > Why could you determine it ? Is PostgreSQL your system ?
> > > >
> > > > Well, if you prefer, we can have a discussion and vote about
> > > > it on pghackers.
> > >
> > > Oh discussion *first* is good but You committed *first*.
> > > So isn't it reasonable to revert your change *first* ?
> > >
> > > This is the second time you disable the on-line reindex
> > > functionality for system tables. Why must I explain the
> > > same thing many times ?
> > 
> > Actually, as a comment here, since I *think* I understand where Tom is
> > coming from ... and since I've either missed it, or it hasn't been
> > answered yet ... why was the original patch incomplete in 
> > only addressing
> > 1 of 3 REINDEX conditions?  Is there a reason why that one condition
> > is/was safe to do it with, but not the other 2?
> 
> Sorry to trouble you.
> 
> In the first place, REINDEX is neither an SQL standard command nor
> a preferable one.
> 
> When I introduced REINDEX command before 7.0, it was not
> transaction-safe and only allowed to call under standalone mode
> essentially.
> 
> Before 7.1, the introduction of pg_class.relfilenode gave us a possibilty
> to make REINDEX command transaction-safe and I tried to make
> REINDEX available under postmaster and the result was
>    1.All user indexes/tables could be REINDEXed under postmaster
>    2.System tables except shared or nailed ones could be REINDEXed
>      under postmaster.
> 
> Note that we couldn't reindex all system tables under postmaster.
> It's the main reason why I didn't implement REINDEX DATABASE
> under postmaster. As for REINDEX, I have 
> 
> Tue Nov 20 02:46:13 2001 UTC (22 months ago)  Tom committed
> the following change which disables the functionality to reindex
> system tables under postmaster.
>      Some minor tweaks of REINDEX processing: grab exclusive
>      lock a little earlier, make error checks more uniform.
> 
> The above change was the first time he disables the functionality. 
> I noticed the change, complained to him and
> Thu Feb 7 00:27:30 2002 UTC (19 months, 1 week ago) I  
>    Removed a check for REINDEX TABLE.
> 
> And this is the second time he disables the functionality without
> any notification to me. Honestly I don't understand why I have to
> explain this kind of thing only so as to revert the change.
> 
> regards,
> Hiroshi Inoue
>  
> 
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 2: you can get off all lists at once with the unregister command
>     (send "unregister YourEmailAddressHere" to majordomo@postgresql.org)
> 

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Wed, 24 Sep 2003 14:08:11 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: pgsql-server/src/backend catalog/index.c comma ...
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Where are we on this?  Does the code path now make sense, at least?

I committed the fixes a few minutes ago.  I now actually believe that
reindexing system tables works ;-) ... hopefully Hiroshi does too.

			regards, tom lane

Wed, 24 Sep 2003 15:11:19 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pgsql-server/src/backend catalog/index.c comma ... 
> -----Original Message-----
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us] 
> 
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Where are we on this?  Does the code path now make sense, at least?
> 
> I committed the fixes a few minutes ago.  I now actually believe that
> reindexing system tables works ;-) ... hopefully Hiroshi does too.

I don't see your commit message yet.

Anyway I I'm not completely agree with your proposal or patch.
Different from you, I'm far from full time developper and I haven't
have much time to check your proposal or patch. 
Is it wrong to believe I have a right to conitune the discussion ?

regards,
Hiroshi Inoue


Thu, 25 Sep 2003 06:52:58 +0900
"Hiroshi Inoue" <inoue@tpf.co.jp>
Re: pgsql-server/src/backend catalog/index.c comma ... 
Hiroshi Inoue wrote:
> > -----Original Message-----
> > From: Tom Lane [mailto:tgl@sss.pgh.pa.us] 
> > 
> > Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > > Where are we on this?  Does the code path now make sense, at least?
> > 
> > I committed the fixes a few minutes ago.  I now actually believe that
> > reindexing system tables works ;-) ... hopefully Hiroshi does too.
> 
> I don't see your commit message yet.
> 
> Anyway I I'm not completely agree with your proposal or patch.
> Different from you, I'm far from full time developper and I haven't
> have much time to check your proposal or patch. 
> Is it wrong to believe I have a right to conitune the discussion ?

Please continue the discussion, but Hiroshi, we need you to look at the
code and give us your ideas.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Wed, 24 Sep 2003 18:03:45 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: pgsql-server/src/backend catalog/index.c comma ...
"Hiroshi Inoue" <inoue@tpf.co.jp> writes:
>> I committed the fixes a few minutes ago.  I now actually believe that
>> reindexing system tables works ;-) ... hopefully Hiroshi does too.

> I don't see your commit message yet.

CVS reports to the committers list seem to be wedged this afternoon :-(.
I already complained to Marc about it.  But in the meantime here is the
cvs2cl entry:

2003-09-24 14:54  tgl

	* doc/src/sgml/ref/postgres-ref.sgml,
	doc/src/sgml/ref/reindex.sgml, src/backend/access/index/genam.c,
	src/backend/access/transam/xact.c, src/backend/catalog/index.c,
	src/backend/catalog/pg_largeobject.c,
	src/backend/commands/functioncmds.c,
	src/backend/commands/indexcmds.c, src/backend/commands/vacuum.c,
	src/backend/executor/execUtils.c,
	src/backend/executor/nodeIndexscan.c,
	src/backend/storage/ipc/sinval.c, src/backend/tcop/postgres.c,
	src/backend/tcop/utility.c, src/backend/utils/cache/relcache.c,
	src/backend/utils/cache/syscache.c,
	src/backend/utils/init/miscinit.c, src/include/miscadmin.h,
	src/include/catalog/index.h, src/include/utils/errcodes.h,
	src/include/utils/rel.h, src/include/utils/relcache.h: Repair some
	REINDEX problems per recent discussions.  The relcache is now able
	to cope with assigning new relfilenode values to nailed-in-cache
	indexes, so they can be reindexed using the fully crash-safe
	method.  This leaves only shared system indexes as special cases. 
	Remove the 'index deactivation' code, since it provides no useful
	protection in the shared- index case.  Require reindexing of shared
	indexes to be done in standalone mode, but remove other
	restrictions on REINDEX.  -P (IgnoreSystemIndexes) now prevents
	using indexes for lookups, but does not disable index updates.	It
	is therefore safe to allow from PGOPTIONS.  Upshot: reindexing
	system catalogs can be done without a standalone backend for all
	cases except shared catalogs.

It's essentially the same as what I posted two days ago, except I
thought it would be a good idea to make REINDEX TABLE take care of the
associated TOAST table too, rather than expecting users to deal with
the toast table separately.


> Is it wrong to believe I have a right to conitune the discussion ?

Not at all, but I already waited two days for your response, and I can't
put development on hold indefinitely.  For now, those changes are in,
and you are welcome to suggest further improvements at your leisure.

			regards, tom lane

Wed, 24 Sep 2003 18:12:58 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pgsql-server/src/backend catalog/index.c comma ... 
> -----Original Message-----
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us] 
> 
> "Hiroshi Inoue" <inoue@tpf.co.jp> writes:
> >> I committed the fixes a few minutes ago.  I now actually 
> believe that
> >> reindexing system tables works ;-) ... hopefully Hiroshi does too.
> 
> > I don't see your commit message yet.
> 
> CVS reports to the committers list seem to be wedged this 
> afternoon :-(.
> I already complained to Marc about it.  But in the meantime 
> here is the
> cvs2cl entry:
> 
> 	Remove the 'index deactivation' code, since it provides 
> no useful
> 	protection in the shared- index case. 

Seems a funny reason to me.
Well, do you also remove reltuples or relpages from pg_class ?

As I already mentioned, I don't love the ReindexProcessing global
flag from the first. For example, it's not thread-safe.

> > Is it wrong to believe I have a right to conitune the discussion ?
> 
> Not at all, but I already waited two days for your response,  
> and I can't
> put development on hold indefinitely.

I've spent a pretty much time only to put back your change.
What I've got until now was just a temporary neutral state.
It's not a neutral state now again. Should I be satisfied with it ?

regards,
Hiroshi Inoue


Fri, 26 Sep 2003 00:33:21 +0900
"Hiroshi Inoue" <inoue@tpf.co.jp>
Re: pgsql-server/src/backend catalog/index.c comma ... 
"Hiroshi Inoue" <inoue@tpf.co.jp> writes:
>> Remove the 'index deactivation' code, since it provides 
>> no useful protection in the shared- index case. 

> Seems a funny reason to me.

Well, as you know I never liked that code; modifying permanent on-disk
data didn't seem like a sensible way to protect against intra-transaction
interlock problems.  So when I saw a chance to get rid of it, I thought
we should take it.

> I've spent a pretty much time only to put back your change.

Why?  As far as I know, the modified code does everything you want.

			regards, tom lane

Thu, 25 Sep 2003 20:04:08 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pgsql-server/src/backend catalog/index.c comma ... 
> -----Original Message-----
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> 
> "Hiroshi Inoue" <inoue@tpf.co.jp> writes:
> >> Remove the 'index deactivation' code, since it provides 
> >> no useful protection in the shared- index case. 
> 
> > Seems a funny reason to me.
> 
> Well, as you know I never liked that code; modifying permanent on-disk
> data didn't seem like a sensible way to protect against intra-transaction
> interlock problems.  So when I saw a chance to get rid of it, I thought
> we should take it.

You dislike it and so what ?
Are you interested in the discussion ?
 
> > I've spent a pretty much time only to put back your change.
> 
> Why?  As far as I know, the modified code does everything you want.

How do you what I want ?
In the first place, there has been no discussion like discussion
in this topic. 

regards,
Hiroshi Inoue

Sat, 27 Sep 2003 06:52:44 +0900
"Hiroshi Inoue" <Inoue@tpf.co.jp>
Re: pgsql-server/src/backend catalog/index.c comma ... 
"Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
>> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
>> Why?  As far as I know, the modified code does everything you want.

> How do you what I want ?

If you aren't going to tell me, I guess I won't know :-(.  It seems
like you want to turn this into some kind of personal issue.  If you
want to tell me what technical problems you see with this code, I'm
very willing to listen.

			regards, tom lane

Fri, 26 Sep 2003 18:08:12 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pgsql-server/src/backend catalog/index.c comma ... 
> -----Original Message-----
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us] 
> 
> "Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> >> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> >> Why?  As far as I know, the modified code does everything you want.
> 
> > How do you what I want ?
> 
> If you aren't going to tell me, I guess I won't know :-(.  It seems
> like you want to turn this into some kind of personal issue.

Personal issue ? 
Are you going to dodge the issue ?

regards,
Hiroshi Inoue


Sun, 28 Sep 2003 07:04:58 +0900
"Hiroshi Inoue" <inoue@tpf.co.jp>
Re: pgsql-server/src/backend catalog/index.c comma ... 

Tom, would you summarize what REINDEX currently _doesn't_ do?

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

Tom Lane wrote:
> I've been looking at the issues involved in reindexing system tables,
> and I now have what I think is a fairly defensible set of proposals.
> 
> We should whenever possible use the same reindexing technique used by
> CLUSTER: assign a new relfilenode number, build the new index in that
> file, and apply an ordinary heap_update operation to update the index's
> pg_class row with the new relfilenode value.  This technique is fully
> crash-safe and transaction-safe (meaning it can be rolled back even after
> completion, in case of failure later in the same transaction).  However,
> there are several pitfalls for applying it to system indexes:
> 
> 1. There is a problem for a shared index, because we have no way to
> update pg_class.relfilenode in all the other databases besides ours.
> I see no good way around this problem.
> 
> 2. There is a problem for a nailed-in-cache index, because the relcache
> isn't prepared to cope with relfilenode updates for such indexes.
> However, that is fixable.
> 
> 3. There is a problem for an index on pg_class itself: doing heap_update
> on a pg_class row must make new index entries.  We have to be careful
> that the new row does appear in the updated index, while not making
> entries in old-and-possibly-broken indexes.  This is doable.
> 
> 4. There is a problem with updating indexes that might be used for catalog
> fetches executed during the index_build process: if we try to use the
> partially-rebuilt index for such a fetch we will fail.  In the case of
> disaster recovery the problem doesn't exist because we'll have
> IsIgnoringSystemIndexes true, but for an ordinary on-line indexing
> operation there's definitely a risk.  Presently the code relies on
> "deactivating indexes" to prevent this, but I think it can be done more
> simply, because we only need to suppress access to the target index
> locally in the reindexing backend.  (Other backends will be locked out by
> means of an AccessExclusiveLock on the system catalog in question.)
> 
> In short then, we can fix things so that the new-relfilenode path can be
> taken in all cases except shared indexes.  For shared indexes, we have
> little choice but to truncate and rebuild the index in-place.  (There is
> then no need to update its pg_class row at all.)  This is of course
> horribly crash-unsafe.
> 
> The code presently uses a "deactivated indexes" flag (namely, setting
> pg_class.relhasindex false) to provide some protection against a crash
> midway through an in-place reindex.  However, this concept is really quite
> useless for a shared index, because only one database could contain the
> deactivation flag.  Accesses from any other database would still try to
> use the broken index.
> 
> Based on this analysis, I feel that the "deactivated indexes" mechanism
> is of no further value and should be retired.  We should instead simply
> acknowledge that reindexing shared indexes is dangerous.  I propose that
> it be allowed only from a standalone backend.  Specifically I think that
> REINDEX INDEX and REINDEX TABLE should error out if pointed to a shared
> index or table, unless running in a standalone backend; REINDEX DATABASE
> should skip over the shared indexes unless running in a standalone
> backend.  (I'm not convinced that either -O or -P needs to be insisted on
> by REINDEX, btw, but we can discuss that separately.)  We'll have to warn
> users not to try to restart the database when reindexing of a shared table
> wasn't successfully completed.
> 
> Details to back up the above claims:
> 
> Part of the code needed to fix the relcache restriction on nailed indexes
> is already there, but ifdef'd out; that's the part that re-reads the
> index's pg_class row after receiving SI inval for it.  There are some
> cases not yet handled though.  In the first place, if the nailed index
> being modified is pg_class_oid_index, ScanPgRelation will try to use that
> same index to load the updated row, and will fail because it'll be trying
> to use the old relfilenode.  We can easily force a seqscan in that case,
> however.  A more subtle problem is that if an SI buffer overflow occurs, 
> RelationCacheInvalidate walks through the relation cache in a random
> order.  I believe that the correct update order has to be first
> pg_class_oid_index, then the other nailed indexes, and finally all other
> relation cache entries.  pg_class_oid_index has to be updated first (with
> the aforementioned seqscan), since it will be used by ScanPgRelation to
> reread the pg_class rows for the other nailed indexes.  Only when we are
> sure the nailed indexes are up-to-date can we safely rebuild other
> relcache entries.
> 
> Assigning a new relfilenode to indexes of pg_class is not a big deal when
> we don't have an index-corruption problem.  We simply have to make the new
> heap_updated row before we start index_build (which indeed the code does
> already); both old and new rows will be indexed in the new index, and all
> is well.  However, if we suspect index corruption then it is important not
> to try to make entries into the old indexes, because we might crash trying
> to update a corrupt index.  I propose the following behavior:
> 
> 	REINDEX INDEX: no special action; this is not the thing to use
> 	when index corruption is suspected anyway.
> 
> 	REINDEX TABLE: while operating on pg_class, arrange for the
> 	relcache's list of known indexes of pg_class to contain only the
> 	indexes already reindexed.  In this way, only those indexes will
> 	be updated by CatalogUpdateIndexes(), and so no untrustworthy
> 	indexes will be touched while making new pg_class rows.
> 
> 	REINDEX DATABASE: take care to process pg_class first.
> 
> To avoid catalog accesses via an index that's being rebuilt, we can simply
> generalize the SetReindexProcessing() state to include the OID of the
> index currently being rebuilt.  Places that presently make checks for
> IsIgnoringSystemIndexes can check this as well and fall back to seqscans
> when the targeted index would have been used.
> 
> One other point: IsIgnoringSystemIndexes seems presently to be taken to
> mean not only that we don't *read* the system indexes, but that we don't
> *write* them either.  I think this is horribly dangerous; it effectively
> means that the only thing you can safely do in a backend started with -P
> is REINDEX.  If you make any other changes to the system catalogs then
> you've re-corrupted the indexes.  Also, you don't have any protection
> against duplicate entries getting made, since the unique indexes are
> disabled.  It would be a lot safer to define IsIgnoringSystemIndexes as
> preventing the system indexes from being used for lookups, while still
> updating the indexes on any catalog modification.  This will not affect
> the safety of REINDEX and will make other operations much less prone to
> pilot error.
> 
> Comments?
> 
> 			regards, tom lane
> 
> ---------------------------(end of broadcast)---------------------------
> TIP 9: the planner will ignore your desire to choose an index scan if your
>       joining column's datatypes do not match
> 

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Sat, 27 Sep 2003 18:05:13 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Improving REINDEX for system indexes (long)
Hiroshi Inoue wrote:
> > -----Original Message-----
> > From: Tom Lane [mailto:tgl@sss.pgh.pa.us] 
> > 
> > "Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> > >> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> > >> Why?  As far as I know, the modified code does everything you want.
> > 
> > > How do you what I want ?
> > 
> > If you aren't going to tell me, I guess I won't know :-(.  It seems
> > like you want to turn this into some kind of personal issue.
> 
> Personal issue ? 
> Are you going to dodge the issue ?

Hiroshi, Tom already apologized for doing this without prior discussion.
What open issues are there?

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Sat, 27 Sep 2003 18:22:52 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: pgsql-server/src/backend catalog/index.c comma ...
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> Tom, would you summarize what REINDEX currently _doesn't_ do?

As of CVS tip I think the only deficiency is that indexes on the shared
catalogs (pg_database, pg_shadow, pg_group) have to be reindexed in
place, rather than being rebuilt with a new relfilenode as is done for
CLUSTER or TRUNCATE.  In-place reindexing isn't crash-safe, since if
you fail you're left with a half-built (effectively corrupt) index.

I don't see any way to avoid that, though, since we cannot change the
relfilenode value for a shared index.

I was toying with the notion of changing btree index build to not write
the metapage until the index is fully built; in this way, at least the
corrupted state of the index would be obvious.  (You'd get "not a btree"
failures.)

			regards, tom lane

Sat, 27 Sep 2003 18:37:22 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Improving REINDEX for system indexes (long) 
Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Tom, would you summarize what REINDEX currently _doesn't_ do?
> 
> As of CVS tip I think the only deficiency is that indexes on the shared
> catalogs (pg_database, pg_shadow, pg_group) have to be reindexed in
> place, rather than being rebuilt with a new relfilenode as is done for
> CLUSTER or TRUNCATE.  In-place reindexing isn't crash-safe, since if
> you fail you're left with a half-built (effectively corrupt) index.
> 
> I don't see any way to avoid that, though, since we cannot change the
> relfilenode value for a shared index.
> 
> I was toying with the notion of changing btree index build to not write
> the metapage until the index is fully built; in this way, at least the
> corrupted state of the index would be obvious.  (You'd get "not a btree"
> failures.)

Oh, that's great. I can't imagine a lot of traffic in those shared
tables anyway.  So you implemented all the ideas in the email --- great.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Sat, 27 Sep 2003 19:51:16 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Improving REINDEX for system indexes (long)
On Sat, Sep 27, 2003 at 06:37:22PM -0400, Tom Lane wrote:
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > Tom, would you summarize what REINDEX currently _doesn't_ do?
> 
> As of CVS tip I think the only deficiency is that indexes on the shared
> catalogs (pg_database, pg_shadow, pg_group) have to be reindexed in
> place, rather than being rebuilt with a new relfilenode as is done for
> CLUSTER or TRUNCATE.  In-place reindexing isn't crash-safe, since if
> you fail you're left with a half-built (effectively corrupt) index.
> 
> I don't see any way to avoid that, though, since we cannot change the
> relfilenode value for a shared index.

What about creating a separate filenode anyway and renaming the files
afterwards?  It would not be an atomic operation anyway, but it would be
better than the current setup IMHO.

(It seems unlikely that one would have > 1GB indexes for shared
relations...)

-- 
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"La espina, desde que nace, ya pincha" (Proverbio africano)

Sat, 27 Sep 2003 20:10:21 -0400
Alvaro Herrera <alvherre@dcc.uchile.cl>
Re: Improving REINDEX for system indexes (long)
Alvaro Herrera <alvherre@dcc.uchile.cl> writes:
> On Sat, Sep 27, 2003 at 06:37:22PM -0400, Tom Lane wrote:
>> I don't see any way to avoid that, though, since we cannot change the
>> relfilenode value for a shared index.

> What about creating a separate filenode anyway and renaming the files
> afterwards?  It would not be an atomic operation anyway, but it would be
> better than the current setup IMHO.

I think it would be difficult to persuade the buffer manager and storage
manager to work with this; from their point of view you'd be moving a
relation underneath them.  I doubt it's really worth the trouble; how
often do you need to reindex a shared catalog?

			regards, tom lane

Sat, 27 Sep 2003 20:15:48 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: Improving REINDEX for system indexes (long) 
Tom Lane wrote:
> > What about creating a separate filenode anyway and renaming the files
> > afterwards?  It would not be an atomic operation anyway, but it would be
> > better than the current setup IMHO.
> 
> I think it would be difficult to persuade the buffer manager and storage
> manager to work with this; from their point of view you'd be moving a
> relation underneath them.  I doubt it's really worth the trouble; how
> often do you need to reindex a shared catalog?

The point I missed originally is that he is talking about shared
catalogs, not system catalogs, which work fine in CVS.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Sat, 27 Sep 2003 20:18:07 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: Improving REINDEX for system indexes (long)
> -----Original Message-----
> From: Bruce Momjian [mailto:pgman@candle.pha.pa.us] 
> 
> Hiroshi Inoue wrote:
> > > -----Original Message-----
> > > From: Tom Lane [mailto:tgl@sss.pgh.pa.us] 
> > > 
> > > "Hiroshi Inoue" <Inoue@tpf.co.jp> writes:
> > > >> From: Tom Lane [mailto:tgl@sss.pgh.pa.us]
> > > >> Why?  As far as I know, the modified code does 
> everything you want.
> > > 
> > > > How do you what I want ?
> > > 
> > > If you aren't going to tell me, I guess I won't know :-(. 
>  It seems
> > > like you want to turn this into some kind of personal issue.
> > 
> > Personal issue ? 
> > Are you going to dodge the issue ?
> 
> Hiroshi, Tom already apologized for doing this without prior 
> discussion.

Though there has been no discussion like discussion on this
item between Tom and me, his code is already there. 
Is it reasonbale ? As I already mentioned many many times
putting back his change should have been the first thing but
it wasn't.

regards,
Hiroshi Inoue


Sun, 28 Sep 2003 18:18:11 +0900
"Hiroshi Inoue" <inoue@tpf.co.jp>
Re: pgsql-server/src/backend catalog/index.c comma ...
Hiroshi Inoue wrote:
> > > Personal issue ? 
> > > Are you going to dodge the issue ?
> > 
> > Hiroshi, Tom already apologized for doing this without prior 
> > discussion.
> 
> Though there has been no discussion like discussion on this
> item between Tom and me, his code is already there. 
> Is it reasonbale ? As I already mentioned many many times
> putting back his change should have been the first thing but
> it wasn't.

That is true --- he should have discussed it first (he apologized), and
put it back when there was objection, but I think he was waiting to find
out what the objection was.  Now that it is done, is there something
that still needs attention.  He said that reindex covers everything but
reindexing of shared relation indexes, which is only pg_database,
pg_user, and pg_group, all of which are low modification tables.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Sun, 28 Sep 2003 11:04:40 -0400 (EDT)
Bruce Momjian <pgman@candle.pha.pa.us>
Re: pgsql-server/src/backend catalog/index.c comma ...
"Hiroshi Inoue" <inoue@tpf.co.jp> writes:
> Though there has been no discussion like discussion on this
> item between Tom and me, his code is already there. 
> Is it reasonbale ? As I already mentioned many many times
> putting back his change should have been the first thing but
> it wasn't.

You already reverted the first version.  I followed full protocol
in the second version: I posted a design spec and then a proposed
patch, and I waited what seemed a reasonable length of time for
comments before committing.  You do not have the right to expect
that commits will be held off indefinitely if you don't have the
time to respond to discussion.

I should also point out that according to the CVS logs, you have not
touched the reindex code in nearly two years.  If you were actively
working on it, I'd surely not have joggled your elbow.  But you can't
expect that there is a permanent KEEP OUT sign on code you once worked
on.

			regards, tom lane

Sun, 28 Sep 2003 11:36:54 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pgsql-server/src/backend catalog/index.c comma ... 
On Sat, Sep 27, 2003 at 08:18:07PM -0400, Bruce Momjian wrote:
> Tom Lane wrote:
> > > What about creating a separate filenode anyway and renaming the files
> > > afterwards?  It would not be an atomic operation anyway, but it would be
> > > better than the current setup IMHO.
> > 
> > I think it would be difficult to persuade the buffer manager and storage
> > manager to work with this; from their point of view you'd be moving a
> > relation underneath them.  I doubt it's really worth the trouble; how
> > often do you need to reindex a shared catalog?
> 
> The point I missed originally is that he is talking about shared
> catalogs, not system catalogs, which work fine in CVS.

Well, my idea was to reduce the window of time during which the index
would be corrupt, i.e. not completely rebuilt.  This is only an issue
with shared indexes, because other system indexes do use the changing
relfilenode thingie already.

However, the main reason for reindexing is a corrupt index, so if for
some reason the new index is also corrupt (e.g. the machine crashes
midway) there's no point in having a separate index filenode anyway.

-- 
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"The eagle never lost so much time, as
when he submitted to learn of the crow." (William Blake)

Sun, 28 Sep 2003 14:15:53 -0400
Alvaro Herrera <alvherre@dcc.uchile.cl>
Re: Improving REINDEX for system indexes (long)
Bruce Momjian wrote:
> 
> Hiroshi Inoue wrote:
> > > > Personal issue ?
> > > > Are you going to dodge the issue ?
> > >
> > > Hiroshi, Tom already apologized for doing this without prior
> > > discussion.
> >
> > Though there has been no discussion like discussion on this
> > item between Tom and me, his code is already there.
> > Is it reasonbale ? As I already mentioned many many times
> > putting back his change should have been the first thing but
> > it wasn't.
> 
> That is true --- he should have discussed it first (he apologized),

As I already mentioned many times, thers hasn't been any
discussion like discussion yet though it should have been
the first.

After all it was me not Tom who put back his first change.
When I did it, he already posted another patch. It seems 
clear to me he saw no necessity to put it back. 

Also note his patch was not applicable after my putting
back the change, He has never posted other applicable patches.

I told him that I need some time to remember my code.
I also tried to reply some points of his proposal though
the trial was far from being sufficient. He suddenly applied
his modified patch without any replies to me.

In my feeling, what he has done is to dodge my questions.
Some people whom I asked have the same feeling.

He also ignored my question about "2 phase commit" in
pgsql-hackers, for example.

Should I be patient with the treatment ?

regards,
Hiroshi Inoue
	http://www.geocities.jp/inocchichichi/psqlodbc/

Mon, 29 Sep 2003 08:50:09 +0900
Hiroshi Inoue <Inoue@tpf.co.jp>
Re: pgsql-server/src/backend catalog/index.c comma ...

On Mon, 29 Sep 2003, Hiroshi Inoue wrote:

> He also ignored my question about "2 phase commit" in pgsql-hackers, for
> example.

Actually, I've been following that thread pretty closely, and I believe I
missed your question :(

Sun, 28 Sep 2003 20:52:44 -0300 (ADT)
"Marc G. Fournier" <scrappy@postgresql.org>
Re: pgsql-server/src/backend catalog/index.c comma ...
"Marc G. Fournier" <scrappy@postgresql.org> writes:
> On Mon, 29 Sep 2003, Hiroshi Inoue wrote:
>> He also ignored my question about "2 phase commit" in pgsql-hackers, for
>> example.

> Actually, I've been following that thread pretty closely, and I believe I
> missed your question :(

[ checks back in thread ]  I didn't respond to that message because
I didn't have anything much to add to the discussion at that point.
It wasn't anything personal; in fact I don't think I particularly
noticed that the message was from Hiroshi as opposed to anyone else
who was participating.

			regards, tom lane

Sun, 28 Sep 2003 20:36:32 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pgsql-server/src/backend catalog/index.c comma ... 
"Marc G. Fournier" wrote:
> 
> On Mon, 29 Sep 2003, Hiroshi Inoue wrote:
> 
> > He also ignored my question about "2 phase commit" in pgsql-hackers, for
> > example.
> 
> Actually, I've been following that thread pretty closely, and I believe I
> missed your question :(

OK I would explain it again.

Bruced asked.

> Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> Tom Lane wrote:
>>> You're not considering the possibility of a transient communication
>>> failure.

>> Can't the master re-send the request after a timeout?

Tom's answer was.

> Not "it can", but "it has to". 

IMHO the partcipants(masters) don't have to bear such
heavy responsibility. My answer is

  Yes "it can", but "it doesn't have to".

Of cource, the cooridnater(slave) has the responsibility
to retry the commit operation for the in-doubt transaction.

regards,
Hiroshi Inoue

Mon, 29 Sep 2003 10:51:09 +0900
Hiroshi Inoue <Inoue@tpf.co.jp>
Re: pgsql-server/src/backend catalog/index.c comma ...


On Mon, 29 Sep 2003, Hiroshi Inoue wrote:

> "Marc G. Fournier" wrote:
> >
> > On Mon, 29 Sep 2003, Hiroshi Inoue wrote:
> >
> > > He also ignored my question about "2 phase commit" in pgsql-hackers, for
> > > example.
> >
> > Actually, I've been following that thread pretty closely, and I believe I
> > missed your question :(
>
> OK I would explain it again.
>
> Bruced asked.
>
> > Bruce Momjian <pgman@candle.pha.pa.us> writes:
> >> Tom Lane wrote:
> >>> You're not considering the possibility of a transient communication
> >>> failure.
>
> >> Can't the master re-send the request after a timeout?
>
> Tom's answer was.
>
> > Not "it can", but "it has to".
>
> IMHO the partcipants(masters) don't have to bear such
> heavy responsibility. My answer is
>
>   Yes "it can", but "it doesn't have to".
>
> Of cource, the cooridnater(slave) has the responsibility
> to retry the commit operation for the in-doubt transaction.

'k, a statement, not a question, which is why it didn't stick with me as
being unanswered :(

Sun, 28 Sep 2003 22:58:15 -0300 (ADT)
"Marc G. Fournier" <scrappy@postgresql.org>
Re: pgsql-server/src/backend catalog/index.c comma ...
Tom Lane wrote:
> 
> "Hiroshi Inoue" <inoue@tpf.co.jp> writes:
> > Though there has been no discussion like discussion on this
> > item between Tom and me, his code is already there.
> > Is it reasonbale ? As I already mentioned many many times
> > putting back his change should have been the first thing but
> > it wasn't.
> 
> You already reverted the first version. I followed full protocol
> in the second version: I posted a design spec and then a proposed
> patch, and I waited what seemed a reasonable length of time for
> comments before committing.

> You do not have the right to expect
> that commits will be held off indefinitely if you don't have the
> time to respond to discussion.

But you were able to tell me when you would commit the
change, weren't you ? In the first place this issue was
started from your mistake and you had to be careful not
to repeat such mistake.
 
> I should also point out that according to the CVS logs, you have not
> touched the reindex code in nearly two years.  If you were actively
> working on it, I'd surely not have joggled your elbow.

As I already mentioned many times, what you did first was
to disable a functionality. AFAIR Bruce or you always asked
the lists about the meaning when removing a code even if the
code is ancient. As for REINDEX, what you only had to do was
to ask me if the change has no problem.

regards,
Hiroshi Inoue
	http://www.geocities.jp/inocchichichi/psqlodbc/

Mon, 29 Sep 2003 19:51:35 +0900
Hiroshi Inoue <Inoue@tpf.co.jp>
Re: pgsql-server/src/backend catalog/index.c comma ...
Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> But you were able to tell me when you would commit the
> change, weren't you ? In the first place this issue was
> started from your mistake and you had to be careful not
> to repeat such mistake.

I already apologized for that error.  I'm not sure what else
you want me to do.

> As for REINDEX, what you only had to do was
> to ask me if the change has no problem.

I posted a proposal and a patch for you to comment on.
So far you've given me no useful feedback on either,
only complaints that are looking more and more like
personal attacks.

			regards, tom lane

Mon, 29 Sep 2003 10:30:40 -0400
Tom Lane <tgl@sss.pgh.pa.us>
Re: pgsql-server/src/backend catalog/index.c comma ... 
> -----Original Message-----
> From: Tom Lane [mailto:tgl@sss.pgh.pa.us] 
> 
> Hiroshi Inoue <Inoue@tpf.co.jp> writes:
> > But you were able to tell me when you would commit the
> > change, weren't you ? In the first place this issue was
> > started from your mistake and you had to be careful not
> > to repeat such mistake.
> 
> I already apologized for that error.  I'm not sure what else
> you want me to do.

Did you tell me when you would commit your patch ?
Sorry if I missed it.
 
> > As for REINDEX, what you only had to do was
> > to ask me if the change has no problem.
> 
> I posted a proposal and a patch for you to comment on.

??? My above comment is a part of my reply to your comment
   > I should also point out that according to the CVS logs, you have not
   > touched the reindex code in nearly two years.

... And so I don't understand what your above reply is for.

regards,
Hiroshi Inoue


Mon, 29 Sep 2003 23:53:47 +0900
"Hiroshi Inoue" <inoue@tpf.co.jp>
Re: pgsql-server/src/backend catalog/index.c comma ... 