Re: pgsql-server: Add ALTER INDEX, particularly for

Log Message:
-----------
Add ALTER INDEX, particularly for moving tablespaces.

Gavin Sherry

Modified Files:
--------------
pgsql-server/src/backend/parser:
gram.y (r2.471 -> r2.472)
(http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/backend/parser/gram.y.diff?r1=2.471&r2=2.472)
pgsql-server/src/backend/tcop:
utility.c (r1.225 -> r1.226)
(http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/backend/tcop/utility.c.diff?r1=1.225&r2=1.226)
pgsql-server/src/bin/psql:
tab-complete.c (r1.109 -> r1.110)
(http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/bin/psql/tab-complete.c.diff?r1=1.109&r2=1.110)
pgsql-server/src/include/nodes:
parsenodes.h (r1.266 -> r1.267)
(http://developer.postgresql.org/cvsweb.cgi/pgsql-server/src/include/nodes/parsenodes.h.diff?r1=1.266&r2=1.267)

Added Files:
-----------
pgsql-server/doc/src/sgml/ref:
alter_index.sgml (r1.1)
(http://developer.postgresql.org/cvsweb.cgi/pgsql-server/doc/src/sgml/ref/alter_index.sgml?rev=1.1&content-type=text/x-cvsweb-markup)

momjian(at)svr1(dot)postgresql(dot)org (Bruce Momjian) writes:
> Add ALTER INDEX, particularly for moving tablespaces.

This patch is a perfect example of why unreviewed patches should not
go in during beta.

So far I have noticed the following problems with it:

* Added reference page wasn't linked into the docs build.

* Added a field to struct AlterTableStmt, but did not do the necessary
housekeeping for extending a Node (eg, copyfuncs and equalfuncs
adjustments), nor make sure the field is validly set in every place
an AlterTableStmt is constructed.

* ALTER INDEX RENAME doesn't actually work.

regression=# alter index foo_pkey rename to zzz;
ERROR: unrecognized rename stmt type: 9

That's not counting the problem someone else already reported with
incorrect tab-completion.

regards, tom lane

Tom Lane wrote:
> momjian(at)svr1(dot)postgresql(dot)org (Bruce Momjian) writes:
> > Add ALTER INDEX, particularly for moving tablespaces.
>
> This patch is a perfect example of why unreviewed patches should not
> go in during beta.
>
> So far I have noticed the following problems with it:
>
> * Added reference page wasn't linked into the docs build.
>
> * Added a field to struct AlterTableStmt, but did not do the necessary
> housekeeping for extending a Node (eg, copyfuncs and equalfuncs
> adjustments), nor make sure the field is validly set in every place
> an AlterTableStmt is constructed.
>
> * ALTER INDEX RENAME doesn't actually work.
>
> regression=# alter index foo_pkey rename to zzz;
> ERROR: unrecognized rename stmt type: 9
>
> That's not counting the problem someone else already reported with
> incorrect tab-completion.

Yes, you are right. Because Gavin is "Mr Tablespaces" I didn't give it
a thorough read.

FYI, I just fixed the tab completion problem.

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

On Sat, 21 Aug 2004, Bruce Momjian wrote:

> Tom Lane wrote:
>> momjian(at)svr1(dot)postgresql(dot)org (Bruce Momjian) writes:
>>> Add ALTER INDEX, particularly for moving tablespaces.
>>
>> This patch is a perfect example of why unreviewed patches should not
>> go in during beta.
>>
>> So far I have noticed the following problems with it:
>>
>> * Added reference page wasn't linked into the docs build.
>>
>> * Added a field to struct AlterTableStmt, but did not do the necessary
>> housekeeping for extending a Node (eg, copyfuncs and equalfuncs
>> adjustments), nor make sure the field is validly set in every place
>> an AlterTableStmt is constructed.
>>
>> * ALTER INDEX RENAME doesn't actually work.
>>
>> regression=# alter index foo_pkey rename to zzz;
>> ERROR: unrecognized rename stmt type: 9
>>
>> That's not counting the problem someone else already reported with
>> incorrect tab-completion.
>
> Yes, you are right. Because Gavin is "Mr Tablespaces" I didn't give it
> a thorough read.
>
> FYI, I just fixed the tab completion problem.

Considering point 3 seems a wee bit critical, at least to me, should the
patch be removed and postponed for a non-Beta period?

----
Marc G. Fournier Hub.Org Networking Services (http://www.hub.org)
Email: scrappy(at)hub(dot)org Yahoo!: yscrappy ICQ: 7615664

"Marc G. Fournier" <scrappy(at)postgresql(dot)org> writes:
> Considering point 3 seems a wee bit critical, at least to me, should the
> patch be removed and postponed for a non-Beta period?

No, the fixes are all trivial. I'm just chewing out Bruce and Gavin
for sloppiness ;-)

regards, tom lane

Umm... oops.

On Sat, 21 Aug 2004, Tom Lane wrote:

> momjian(at)svr1(dot)postgresql(dot)org (Bruce Momjian) writes:
> > Add ALTER INDEX, particularly for moving tablespaces.
>
> This patch is a perfect example of why unreviewed patches should not
> go in during beta.
>
> So far I have noticed the following problems with it:
>
> * Added reference page wasn't linked into the docs build.
>
> * Added a field to struct AlterTableStmt, but did not do the necessary
> housekeeping for extending a Node (eg, copyfuncs and equalfuncs
> adjustments), nor make sure the field is validly set in every place
> an AlterTableStmt is constructed.
>
> * ALTER INDEX RENAME doesn't actually work.
>
> regression=# alter index foo_pkey rename to zzz;
> ERROR: unrecognized rename stmt type: 9

Thanks for spotting this sloppy work. Basically, I cropped my diff because
there was lots of unrelated code in the work space. But I was a little too
hard and culled allfiles.sgml, alter.c, equal/copyfuncs.c and analyze.c.
BUT I should have tested the patch on a fresh checkout, especially since
someone else noticed the tab-completition naming thing. Sorry.

I was getting together a patch against HEAD but I just noticed that you
fixed this. Thanks.

Gavin

Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> Thanks for spotting this sloppy work. Basically, I cropped my diff because
> there was lots of unrelated code in the work space. But I was a little too
> hard and culled allfiles.sgml, alter.c, equal/copyfuncs.c and analyze.c.

I was wondering if something like that might have happened --- I know
you know better on all these points ;-)

> I was getting together a patch against HEAD but I just noticed that you
> fixed this. Thanks.

Yeah, I think everything is fixed in CVS tip. Let me know if there was
anything I missed.

regards, tom lane

Here is a change to the alter_table regression test to use ALTER INDEX ...
RENAME instead of ALTER TABLE ... RENAME in the case of an index renaming.
Its probably not the right place for it, but I'm not sure where is the
right place for it.

Thanks,

Gavin

On Sat, 21 Aug 2004, Tom Lane wrote:

> Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> > Thanks for spotting this sloppy work. Basically, I cropped my diff because
> > there was lots of unrelated code in the work space. But I was a little too
> > hard and culled allfiles.sgml, alter.c, equal/copyfuncs.c and analyze.c.
>
> I was wondering if something like that might have happened --- I know
> you know better on all these points ;-)
>
> > I was getting together a patch against HEAD but I just noticed that you
> > fixed this. Thanks.
>
> Yeah, I think everything is fixed in CVS tip. Let me know if there was
> anything I missed.
>
> regards, tom lane
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo(at)postgresql(dot)org
>
>
> !DSPAM:4127e64e43221930617052!
>
>

>>Thanks for spotting this sloppy work. Basically, I cropped my diff because
>>there was lots of unrelated code in the work space. But I was a little too
>>hard and culled allfiles.sgml, alter.c, equal/copyfuncs.c and analyze.c.
>
>
> I was wondering if something like that might have happened --- I know
> you know better on all these points ;-)

I have a whole separate CVS checkout for every path I'm working on -
makes life a lot easier :)

Not that Tom doesn't still find bugs in my stuff :P

Chris

> I have a whole separate CVS checkout for every path I'm working on -
> makes life a lot easier :)

Dammit - I meant 'patch'.

Chris

I think I missed this in the earlier patches, but does the alter table syntax
still work for indexes in addition to the new syntax?

On Saturday 21 August 2004 22:47, Gavin Sherry wrote:
> Here is a change to the alter_table regression test to use ALTER INDEX ...
> RENAME instead of ALTER TABLE ... RENAME in the case of an index renaming.
> Its probably not the right place for it, but I'm not sure where is the
> right place for it.
>
> Thanks,
>
> Gavin
>
> On Sat, 21 Aug 2004, Tom Lane wrote:
> > Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> > > Thanks for spotting this sloppy work. Basically, I cropped my diff
> > > because there was lots of unrelated code in the work space. But I was a
> > > little too hard and culled allfiles.sgml, alter.c, equal/copyfuncs.c
> > > and analyze.c.
> >
> > I was wondering if something like that might have happened --- I know
> > you know better on all these points ;-)
> >
> > > I was getting together a patch against HEAD but I just noticed that you
> > > fixed this. Thanks.
> >
> > Yeah, I think everything is fixed in CVS tip. Let me know if there was
> > anything I missed.
> >
> > regards, tom lane
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 1: subscribe and unsubscribe commands go to majordomo(at)postgresql(dot)org
> >
> >
> > !DSPAM:4127e64e43221930617052!

--
Robert Treat
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL

Yes

On Tue, 24 Aug 2004, Robert Treat wrote:

> I think I missed this in the earlier patches, but does the alter table syntax
> still work for indexes in addition to the new syntax?
>
> On Saturday 21 August 2004 22:47, Gavin Sherry wrote:
> > Here is a change to the alter_table regression test to use ALTER INDEX ...
> > RENAME instead of ALTER TABLE ... RENAME in the case of an index renaming.
> > Its probably not the right place for it, but I'm not sure where is the
> > right place for it.
> >
> > Thanks,
> >
> > Gavin
> >
> > On Sat, 21 Aug 2004, Tom Lane wrote:
> > > Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> > > > Thanks for spotting this sloppy work. Basically, I cropped my diff
> > > > because there was lots of unrelated code in the work space. But I was a
> > > > little too hard and culled allfiles.sgml, alter.c, equal/copyfuncs.c
> > > > and analyze.c.
> > >
> > > I was wondering if something like that might have happened --- I know
> > > you know better on all these points ;-)
> > >
> > > > I was getting together a patch against HEAD but I just noticed that you
> > > > fixed this. Thanks.
> > >
> > > Yeah, I think everything is fixed in CVS tip. Let me know if there was
> > > anything I missed.
> > >
> > > regards, tom lane
> > >
> > > ---------------------------(end of broadcast)---------------------------
> > > TIP 1: subscribe and unsubscribe commands go to majordomo(at)postgresql(dot)org
> > >
> > >
> > > !DSPAM:4127e64e43221930617052!
>
> --
> Robert Treat
> Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL
>

Shouldn't the regression tests reflect that?

Robert Treat

On Tue, 2004-08-24 at 22:31, Gavin Sherry wrote:
> Yes
>
> On Tue, 24 Aug 2004, Robert Treat wrote:
>
> > I think I missed this in the earlier patches, but does the alter table syntax
> > still work for indexes in addition to the new syntax?
> >
> > On Saturday 21 August 2004 22:47, Gavin Sherry wrote:
> > > Here is a change to the alter_table regression test to use ALTER INDEX ...
> > > RENAME instead of ALTER TABLE ... RENAME in the case of an index renaming.
> > > Its probably not the right place for it, but I'm not sure where is the
> > > right place for it.
> > >
> > > Thanks,
> > >
> > > Gavin
> > >
> > > On Sat, 21 Aug 2004, Tom Lane wrote:
> > > > Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> > > > > Thanks for spotting this sloppy work. Basically, I cropped my diff
> > > > > because there was lots of unrelated code in the work space. But I was a
> > > > > little too hard and culled allfiles.sgml, alter.c, equal/copyfuncs.c
> > > > > and analyze.c.
> > > >
> > > > I was wondering if something like that might have happened --- I know
> > > > you know better on all these points ;-)
> > > >
> > > > > I was getting together a patch against HEAD but I just noticed that you
> > > > > fixed this. Thanks.
> > > >
> > > > Yeah, I think everything is fixed in CVS tip. Let me know if there was
> > > > anything I missed.
> > > >
> > > > regards, tom lane
> > > >
> > > > ---------------------------(end of broadcast)---------------------------
> > > > TIP 1: subscribe and unsubscribe commands go to majordomo(at)postgresql(dot)org
> > > >
> > > >
> > > > !DSPAM:4127e64e43221930617052!
> >
> > --
> > Robert Treat
> > Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL
> >
>
> ---------------------------(end of broadcast)---------------------------
> TIP 3: if posting/reading through Usenet, please send an appropriate
> subscribe-nomail command to majordomo(at)postgresql(dot)org so that your
> message can get through to the mailing list cleanly
--
Build A Brighter Lamp :: Linux Apache {middleware} PostgreSQL

Patch applied. Thanks.

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

Gavin Sherry wrote:
> Here is a change to the alter_table regression test to use ALTER INDEX ...
> RENAME instead of ALTER TABLE ... RENAME in the case of an index renaming.
> Its probably not the right place for it, but I'm not sure where is the
> right place for it.
>
> Thanks,
>
> Gavin
>
> On Sat, 21 Aug 2004, Tom Lane wrote:
>
> > Gavin Sherry <swm(at)linuxworld(dot)com(dot)au> writes:
> > > Thanks for spotting this sloppy work. Basically, I cropped my diff because
> > > there was lots of unrelated code in the work space. But I was a little too
> > > hard and culled allfiles.sgml, alter.c, equal/copyfuncs.c and analyze.c.
> >
> > I was wondering if something like that might have happened --- I know
> > you know better on all these points ;-)
> >
> > > I was getting together a patch against HEAD but I just noticed that you
> > > fixed this. Thanks.
> >
> > Yeah, I think everything is fixed in CVS tip. Let me know if there was
> > anything I missed.
> >
> > regards, tom lane
> >
> > ---------------------------(end of broadcast)---------------------------
> > TIP 1: subscribe and unsubscribe commands go to majordomo(at)postgresql(dot)org
> >
> >
> > !DSPAM:4127e64e43221930617052!
> >
> >

Content-Description:

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 8: explain analyze is your friend

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