Re: ALTER INDEX

Attached is a patch implementing ALTER INDEX <name> [ RENAME | OWNER TO |
SET TABLESPACE ]

I haven't included any regression tests because most of the code should be
well tested by the existing alter table tests.

Thanks,

Gavin

This patch has a fix for a 'thought-o' in the docs.

Gavin

Gavin Sherry wrote:

> Index: src/bin/psql/tab-complete.c
> ===================================================================
> RCS file: /usr/local/cvsroot/pgsql-server/src/bin/psql/tab-complete.c,v
> retrieving revision 1.109
> diff -2 -c -r1.109 tab-complete.c
> *** src/bin/psql/tab-complete.c 28 Jul 2004 14:23:30 -0000 1.109
> --- src/bin/psql/tab-complete.c 13 Aug 2004 06:34:55 -0000
> ***************
> *** 633,637 ****
> {
> static const char *const list_ALTER[] =
> ! {"DATABASE", "GROUP", "SCHEMA", "TABLE", "TRIGGER", "USER", NULL};
>
> COMPLETE_WITH_LIST(list_ALTER);
> --- 633,638 ----
> {
> static const char *const list_ALTER[] =
> ! {"DATABASE", "GROUP", "SCHEMA", "TABLE", "TRIGGER", "USER", "INDEX",
> ! NULL};
>
> COMPLETE_WITH_LIST(list_ALTER);
> ***************
> *** 647,650 ****
> --- 648,661 ----
> COMPLETE_WITH_LIST(list_ALTERDATABASE);
> }
> + /* ALTER INDEX <name> */
> + else if (pg_strcasecmp(prev3_wd, "ALTER") == 0 &&
> + pg_strcasecmp(prev2_wd, "INDEX") == 0)
> + {
> + static const char *const list_ALTERDATABASE[] =
> + {"SET TABLESPACE", "OWNER TO", "RENAME TO", NULL};
> +
> + COMPLETE_WITH_LIST(list_ALTERDATABASE);

minor issue/nit(?): reusing list_ALTERDATABASE for the ALTER INDEX part
looks a little strange ...

Stefan(who could really need some feedback on his own tab-complete patch
*g*)

Oops.

Too much with the ol' cut and paste.

I'm happy to send an updated patch but perhaps the committer, assuming the
patch is accepted, would be kind enough to update for me.

Thanks for reviewing.

Gavin

On Fri, 13 Aug 2004, Stefan Kaltenbrunner wrote:

> Gavin Sherry wrote:
>
>
> > Index: src/bin/psql/tab-complete.c
> > ===================================================================
> > RCS file: /usr/local/cvsroot/pgsql-server/src/bin/psql/tab-complete.c,v
> > retrieving revision 1.109
> > diff -2 -c -r1.109 tab-complete.c
> > *** src/bin/psql/tab-complete.c 28 Jul 2004 14:23:30 -0000 1.109
> > --- src/bin/psql/tab-complete.c 13 Aug 2004 06:34:55 -0000
> > ***************
> > *** 633,637 ****
> > {
> > static const char *const list_ALTER[] =
> > ! {"DATABASE", "GROUP", "SCHEMA", "TABLE", "TRIGGER", "USER", NULL};
> >
> > COMPLETE_WITH_LIST(list_ALTER);
> > --- 633,638 ----
> > {
> > static const char *const list_ALTER[] =
> > ! {"DATABASE", "GROUP", "SCHEMA", "TABLE", "TRIGGER", "USER", "INDEX",
> > ! NULL};
> >
> > COMPLETE_WITH_LIST(list_ALTER);
> > ***************
> > *** 647,650 ****
> > --- 648,661 ----
> > COMPLETE_WITH_LIST(list_ALTERDATABASE);
> > }
> > + /* ALTER INDEX <name> */
> > + else if (pg_strcasecmp(prev3_wd, "ALTER") == 0 &&
> > + pg_strcasecmp(prev2_wd, "INDEX") == 0)
> > + {
> > + static const char *const list_ALTERDATABASE[] =
> > + {"SET TABLESPACE", "OWNER TO", "RENAME TO", NULL};
> > +
> > + COMPLETE_WITH_LIST(list_ALTERDATABASE);
>
> minor issue/nit(?): reusing list_ALTERDATABASE for the ALTER INDEX part
> looks a little strange ...
>
>
> Stefan(who could really need some feedback on his own tab-complete patch
> *g*)
>
>
> !DSPAM:411c802d169118747610806!
>
>

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

http://momjian.postgresql.org/cgi-bin/pgpatches

It will be applied as soon as one of the PostgreSQL committers reviews
and approves it.

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

Stefan Kaltenbrunner wrote:
> Gavin Sherry wrote:
>
>
> > Index: src/bin/psql/tab-complete.c
> > ===================================================================
> > RCS file: /usr/local/cvsroot/pgsql-server/src/bin/psql/tab-complete.c,v
> > retrieving revision 1.109
> > diff -2 -c -r1.109 tab-complete.c
> > *** src/bin/psql/tab-complete.c 28 Jul 2004 14:23:30 -0000 1.109
> > --- src/bin/psql/tab-complete.c 13 Aug 2004 06:34:55 -0000
> > ***************
> > *** 633,637 ****
> > {
> > static const char *const list_ALTER[] =
> > ! {"DATABASE", "GROUP", "SCHEMA", "TABLE", "TRIGGER", "USER", NULL};
> >
> > COMPLETE_WITH_LIST(list_ALTER);
> > --- 633,638 ----
> > {
> > static const char *const list_ALTER[] =
> > ! {"DATABASE", "GROUP", "SCHEMA", "TABLE", "TRIGGER", "USER", "INDEX",
> > ! NULL};
> >
> > COMPLETE_WITH_LIST(list_ALTER);
> > ***************
> > *** 647,650 ****
> > --- 648,661 ----
> > COMPLETE_WITH_LIST(list_ALTERDATABASE);
> > }
> > + /* ALTER INDEX <name> */
> > + else if (pg_strcasecmp(prev3_wd, "ALTER") == 0 &&
> > + pg_strcasecmp(prev2_wd, "INDEX") == 0)
> > + {
> > + static const char *const list_ALTERDATABASE[] =
> > + {"SET TABLESPACE", "OWNER TO", "RENAME TO", NULL};
> > +
> > + COMPLETE_WITH_LIST(list_ALTERDATABASE);
>
> minor issue/nit(?): reusing list_ALTERDATABASE for the ALTER INDEX part
> looks a little strange ...
>
>
> Stefan(who could really need some feedback on his own tab-complete patch
> *g*)
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo(at)postgresql(dot)org
>

--
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

Patch applied. Thanks.

I originally thought of this as a feature addition, but I realized that
ALTER INDEX is being added because people are going to want to move
tablespaces for indexes, and without this, they can't easily.

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

Gavin Sherry wrote:
> This patch has a fix for a 'thought-o' in the docs.
>
> Gavin

Content-Description:

[ Attachment, skipping... ]

>
> ---------------------------(end of broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings

--
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

I have made this adjustment.

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

Stefan Kaltenbrunner wrote:
> Gavin Sherry wrote:
>
>
> > Index: src/bin/psql/tab-complete.c
> > ===================================================================
> > RCS file: /usr/local/cvsroot/pgsql-server/src/bin/psql/tab-complete.c,v
> > retrieving revision 1.109
> > diff -2 -c -r1.109 tab-complete.c
> > *** src/bin/psql/tab-complete.c 28 Jul 2004 14:23:30 -0000 1.109
> > --- src/bin/psql/tab-complete.c 13 Aug 2004 06:34:55 -0000
> > ***************
> > *** 633,637 ****
> > {
> > static const char *const list_ALTER[] =
> > ! {"DATABASE", "GROUP", "SCHEMA", "TABLE", "TRIGGER", "USER", NULL};
> >
> > COMPLETE_WITH_LIST(list_ALTER);
> > --- 633,638 ----
> > {
> > static const char *const list_ALTER[] =
> > ! {"DATABASE", "GROUP", "SCHEMA", "TABLE", "TRIGGER", "USER", "INDEX",
> > ! NULL};
> >
> > COMPLETE_WITH_LIST(list_ALTER);
> > ***************
> > *** 647,650 ****
> > --- 648,661 ----
> > COMPLETE_WITH_LIST(list_ALTERDATABASE);
> > }
> > + /* ALTER INDEX <name> */
> > + else if (pg_strcasecmp(prev3_wd, "ALTER") == 0 &&
> > + pg_strcasecmp(prev2_wd, "INDEX") == 0)
> > + {
> > + static const char *const list_ALTERDATABASE[] =
> > + {"SET TABLESPACE", "OWNER TO", "RENAME TO", NULL};
> > +
> > + COMPLETE_WITH_LIST(list_ALTERDATABASE);
>
> minor issue/nit(?): reusing list_ALTERDATABASE for the ALTER INDEX part
> looks a little strange ...
>
>
> Stefan(who could really need some feedback on his own tab-complete patch
> *g*)
>
> ---------------------------(end of broadcast)---------------------------
> TIP 1: subscribe and unsubscribe commands go to majordomo(at)postgresql(dot)org
>

--
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 Fri, 20 Aug 2004, Bruce Momjian wrote:

>
> Patch applied. Thanks.
>
> I originally thought of this as a feature addition, but I realized that
> ALTER INDEX is being added because people are going to want to move
> tablespaces for indexes, and without this, they can't easily.

Which would fall under adding a feature onto the tablespaces, not fixing a
bug in tablespaces itself ... does *not* having ALTER INDEX *break*
tablespaces? Causes it not to work, or not build?

>
> ---------------------------------------------------------------------------
>
>
> Gavin Sherry wrote:
>> This patch has a fix for a 'thought-o' in the docs.
>>
>> Gavin
>
> Content-Description:
>
> [ Attachment, skipping... ]
>
>>
>> ---------------------------(end of broadcast)---------------------------
>> TIP 7: don't forget to increase your free space map settings
>
> --
> 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
>
> ---------------------------(end of broadcast)---------------------------
> TIP 7: don't forget to increase your free space map settings
>

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

Marc G. Fournier wrote:
> On Fri, 20 Aug 2004, Bruce Momjian wrote:
>
> >
> > Patch applied. Thanks.
> >
> > I originally thought of this as a feature addition, but I realized that
> > ALTER INDEX is being added because people are going to want to move
> > tablespaces for indexes, and without this, they can't easily.
>
> Which would fall under adding a feature onto the tablespaces, not fixing a
> bug in tablespaces itself ... does *not* having ALTER INDEX *break*
> tablespaces? Causes it not to work, or not build?

No, but it is a missing capability many will complain about. I can
easily remove it. I saw no one comment when I added it to the patches
queue.

--
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

Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> No, but it is a missing capability many will complain about. I can
> easily remove it. I saw no one comment when I added it to the patches
> queue.

I hadn't seen you add it to the patches queue ...

I did see Gavin's submission but did not yet have time to look at the
details. What does it *do* exactly --- simply allow INDEX as a
substitute for TABLE in the syntax, or more? I'm not thrilled at the
idea of adding a lot of duplicate coding for this.

regards, tom lane

On Fri, 20 Aug 2004, Tom Lane wrote:

> Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > No, but it is a missing capability many will complain about. I can
> > easily remove it. I saw no one comment when I added it to the patches
> > queue.
>
> I hadn't seen you add it to the patches queue ...
>
> I did see Gavin's submission but did not yet have time to look at the
> details. What does it *do* exactly --- simply allow INDEX as a
> substitute for TABLE in the syntax, or more? I'm not thrilled at the
> idea of adding a lot of duplicate coding for this.

I tried to avoid any duplication. The patch still uses all the ALTER TABLE
code. Its just a grammar modification and some setting of completion tags.

That being said, I felt obliged to provide at patch when I started hearing
noise about ALTER TABLE <index name> being a bit of a hack -- which it is.

Gavin

Gavin Sherry wrote:
> On Fri, 20 Aug 2004, Tom Lane wrote:
>
> > Bruce Momjian <pgman(at)candle(dot)pha(dot)pa(dot)us> writes:
> > > No, but it is a missing capability many will complain about. I can
> > > easily remove it. I saw no one comment when I added it to the patches
> > > queue.
> >
> > I hadn't seen you add it to the patches queue ...
> >
> > I did see Gavin's submission but did not yet have time to look at the
> > details. What does it *do* exactly --- simply allow INDEX as a
> > substitute for TABLE in the syntax, or more? I'm not thrilled at the
> > idea of adding a lot of duplicate coding for this.
>
> I tried to avoid any duplication. The patch still uses all the ALTER TABLE
> code. Its just a grammar modification and some setting of completion tags.
>
> That being said, I felt obliged to provide at patch when I started hearing
> noise about ALTER TABLE <index name> being a bit of a hack -- which it is.

The issue was that few people currently modify indexes because in the
past you could only rename an index or change its owner. With
tablespaces, we are going to have lots more people moving indexes
around, and I think people were getting confused because there was no
ALTER INDEX to move them.

I can still back it out and leave it for 8.1 but it probably will reduce
confusion and perhaps need for an FAQ, "How do I move an index between
tablespaces?"

FYI, I just fixed a typo in the ALTER INDEX manual page that mentioned
INDEXSPACE instead of TABLESPACE.

--
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