ALTER TABLE OWNER: change indexes

Neil Conway <nconway@klamath.dyndns.org> writes:

Also, I needed some utility code to find the name of a relation, given
its OID.

get_rel_name in utils/cache/lsyscache.c.

I find a suitable utility function in
src/backend/utils/adt/ruleutils.c -- however, it's a static function.

Oh? Looks like someone forgot about lsyscache ...

In general, lsyscache is the place for quick and dirty little catalog
access functions like this.

regards, tom lane

On Sun, 2002-02-24 at 16:35, Tom Lane wrote:

Neil Conway <nconway@klamath.dyndns.org> writes:

Also, I needed some utility code to find the name of a relation, given
its OID.

get_rel_name in utils/cache/lsyscache.c.

Thanks Tom.

I find a suitable utility function in
src/backend/utils/adt/ruleutils.c -- however, it's a static function.

Oh? Looks like someone forgot about lsyscache ...

I've attached a new version of the patch that uses get_rel_name() -- I
also removed get_relation_name() from ruleutils.c and changed that file
to use get_rel_name().

Cheers,

Neil

--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC

BTW, I forgot to inquire about *why* you needed to look up a relation
name. As things stand I think you've coded it okay, but this will need
to change for schemas: a relation name isn't going to be a unique
identifier much longer.

It'd probably be best to redesign the ALTER TABLE routines so that
the recursive execution routine accepts a relation OID rather than a
relation name, with a front-end routine that does a one-time name-to-
OID lookup. Recursion using OID will be simpler than recursion using
name, for both child-table and index cases. And it won't break for
schemas.

Perhaps this could be done as part of the overall refactoring of the
ALTER code that someone (I forget who) was going to look at doing.

This doesn't need to be done just to make this patch acceptable, but
I thought I'd better mention that it needs to be done soon.

Another point that maybe does need immediate attention: as coded,
reassignment of ownership of a table won't affect the associated
TOAST table, if any. Should it?

regards, tom lane

Tom Lane writes:

Another point that maybe does need immediate attention: as coded,
reassignment of ownership of a table won't affect the associated
TOAST table, if any. Should it?

Maybe indexes and TOAST tables shouldn't have ownership it all. (Set
relowner to 0.) Since only owners can create these things, there doesn't
seem to be a point in maintaining ownership.

--
Peter Eisentraut peter_e@gmx.net

Peter Eisentraut <peter_e@gmx.net> writes:

Maybe indexes and TOAST tables shouldn't have ownership it all. (Set
relowner to 0.) Since only owners can create these things, there doesn't
seem to be a point in maintaining ownership.

That's a good thought. At one time we allowed non-owners to create
indexes on tables, but it was correctly pointed out that this was a
bad idea (think UNIQUE index, or a functional index that always
causes an error...). I can't see a real good reason for either indexes
or TOAST tables to have a concept of ownership distinct from the parent
table.

psql's \d displays might have some trouble with this, and pg_dump might
get confused too. But fixing them would be a net improvement in
robustness so it's probably a reasonable thing to do.

regards, tom lane

On Sun, 2002-02-24 at 17:43, Tom Lane wrote:

It'd probably be best to redesign the ALTER TABLE routines so that
the recursive execution routine accepts a relation OID rather than a
relation name, with a front-end routine that does a one-time name-to-
OID lookup. Recursion using OID will be simpler than recursion using
name, for both child-table and index cases. And it won't break for
schemas.

You mentioned child-tables: should ALTER TABLE OWNER also recurse for
those?

BTW, a question on coding style: are large source files discouraged? I
usually like to separate my source into relatively small files, but
there are a bunch of files in the tree with 2000, 3000 or more lines of
code. Is this the preferred style, or would smaller files (divided in
logical groupings of functions, of course) be better?

Perhaps this could be done as part of the overall refactoring of the
ALTER code that someone (I forget who) was going to look at doing.

Do you have any more information on this? (e.g. an ML thread) I could
pick up this work if it's been dropped...

Another point that maybe does need immediate attention: as coded,
reassignment of ownership of a table won't affect the associated
TOAST table, if any. Should it?

Dunno, I don't know anything about TOAST. I would think if anyone would
know, it'd be you ;-)

The revised patch is attached. It follows Tom's suggestion and does
recursion using the OID of the relation (and the sysid of the user, as
well). Since this involved basically rewriting ALTER TABLE OWNER, I may
have messed something up -- but it works, AFAICT.

Now that I've changed ALTER TABLE OWNER to use recursion-with-oid,
should I make changes to the other ALTER TABLE functions as necessary?

Comments and criticism are welcome.

Cheers,

Neil

--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC

Neil Conway <nconway@klamath.dyndns.org> writes:

You mentioned child-tables: should ALTER TABLE OWNER also recurse for
those?

Only if you want to put in an "ONLY" variant to suppress the recursion.
(This might actually be a reasonable thing to do, if so. But I don't
have a strong feeling about it.)

BTW, a question on coding style: are large source files discouraged?

Personally I prefer source file == logical module, however big that is.
For stuff like this it's not always clear where the module boundaries
should be drawn, of course. I really hate the style where each C
function lives in a separate source file; beyond that it's all a matter
of taste to some extent.

My thought would be to group all the ALTER TABLE variants into one file,
perhaps "alter.c", separate from the other current inhabitants of
command.c. Note that alter.c could be a lot smaller than the current
sum of the ALTER routine sizes, given appropriate refactoring to
eliminate duplicate code.

Perhaps this could be done as part of the overall refactoring of the
ALTER code that someone (I forget who) was going to look at doing.

Do you have any more information on this? (e.g. an ML thread) I could
pick up this work if it's been dropped...

I seem to recall discussing this with Gavin Sherry or Christopher
Kings-Lynne sometime in the past six months or so, but can't find the
thread at the moment. AFAIR it hadn't really progressed beyond the
observation that there was an awful lot of duplicated code in those
routines, and that the ones that weren't duplicating it were a few
bricks shy of a load (eg, didn't support recursion but should).
So we were wondering about ways to consolidate the duplication into
some sort of common control routine.

Another point that maybe does need immediate attention: as coded,
reassignment of ownership of a table won't affect the associated
TOAST table, if any. Should it?

Dunno, I don't know anything about TOAST. I would think if anyone would
know, it'd be you ;-)

Well, see Peter's suggestion that this is all wrong because indexes
don't have meaningful ownership anyway. I think he's got a point...

regards, tom lane

On Sun, 2002-02-24 at 22:37, Tom Lane wrote:

Neil Conway <nconway@klamath.dyndns.org> writes:

You mentioned child-tables: should ALTER TABLE OWNER also recurse for
those?

Only if you want to put in an "ONLY" variant to suppress the recursion.
(This might actually be a reasonable thing to do, if so. But I don't
have a strong feeling about it.)

Okay, I'll do that in a later patch.

My thought would be to group all the ALTER TABLE variants into one file,
perhaps "alter.c", separate from the other current inhabitants of
command.c. Note that alter.c could be a lot smaller than the current
sum of the ALTER routine sizes, given appropriate refactoring to
eliminate duplicate code.

Okay, I'll create alter.c and see if I can refactor some of the ALTER
code -- but that can wait for a later patch also.

Another point that maybe does need immediate attention: as coded,
reassignment of ownership of a table won't affect the associated
TOAST table, if any. Should it?

Dunno, I don't know anything about TOAST. I would think if anyone would
know, it'd be you ;-)

Well, see Peter's suggestion that this is all wrong because indexes
don't have meaningful ownership anyway. I think he's got a point...

That's probably true -- in the long-run, it probably makes more sense to
remove the concept of ownership from indexes.

However, in the mean-time, I think my patch is still valid. Unless there
are any remaining problems, please apply for 7.3.

Cheers,

Neil

--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC

Neil Conway <nconway@klamath.dyndns.org> writes:

Well, see Peter's suggestion that this is all wrong because indexes
don't have meaningful ownership anyway. I think he's got a point...

That's probably true -- in the long-run, it probably makes more sense to
remove the concept of ownership from indexes.

However, in the mean-time, I think my patch is still valid. Unless there
are any remaining problems, please apply for 7.3.

No, I disagree. If we are intending to remove ownership from indexes,
there is no good reason to add code that will only get taken out again.
There is no bug here of sufficient importance to warrant a temporary
hack solution; the existing behavior can be lived with.

regards, tom lane

On Mon, 2002-02-25 at 18:32, Tom Lane wrote:

However, in the mean-time, I think my patch is still valid. Unless there
are any remaining problems, please apply for 7.3.

No, I disagree. If we are intending to remove ownership from indexes,
there is no good reason to add code that will only get taken out again.

Well, that's assuming that someone steps forward to actually write the
code. I haven't heard that anyone has...

And as for adding code that will only be removed, the code is finished,
tested, reviewed and ready to apply -- all the work _has_ been done.

Additionally, if someone eventually fixes the index-ownership situation,
the changes to command.c to remove the index recursion are trivial. This
patch also includes some refactoring and code cleanups that are useful
in any case.

There is no bug here of sufficient importance to warrant a temporary
hack solution; the existing behavior can be lived with.

I wouldn't call my solution a "temporary hack"; it's a similar idea to
code that is found throughout the tree.

Cheers,

Neil

--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC

Neil Conway <nconway@klamath.dyndns.org> writes:

Additionally, if someone eventually fixes the index-ownership situation,
the changes to command.c to remove the index recursion are trivial.

... but won't necessarily get done. More to the point, they may confuse
someone who's trying to refactor the code: without careful thought, he
might think he needs to support recursion over indexes as well as child
tables.

This patch also includes some refactoring and code cleanups that are
useful in any case.

Sure. Please resubmit just that part.

regards, tom lane

Neil Conway writes:

Okay, I'll create alter.c and see if I can refactor some of the ALTER
code -- but that can wait for a later patch also.

It might make more sense to factor the source files by the kind of objects
they act on. For instance, the CREATE/ALTER USER code shares a lot of
code, but it shares zero with, say, ALTER TABLE.

--
Peter Eisentraut peter_e@gmx.net

On Mon, 2002-02-25 at 18:48, Tom Lane wrote:

Neil Conway <nconway@klamath.dyndns.org> writes:

Additionally, if someone eventually fixes the index-ownership situation,
the changes to command.c to remove the index recursion are trivial.

... but won't necessarily get done. More to the point, they may confuse
someone who's trying to refactor the code: without careful thought, he
might think he needs to support recursion over indexes as well as child
tables.

Not if the code includes a comment (as it does) that the recursion is
intended _only_ to support changing the ownership of any indexes which
belong to the table.

IMHO, it's not confusing at all: in the current code, indexes have
owners, and should be owned by the owner of the table they belong to.
The patch makes this consistent; without the patch, one might conclude
that there are reasonable situations in the owner of a table should not
own its indexes, which is incorrect AFAIK.

BTW, should ownership be removed from sequences as well?

This patch also includes some refactoring and code cleanups that are
useful in any case.

Sure. Please resubmit just that part.

Okay, I've attached a patch which implements this.

I think it is still a bad idea to leave code that is _known_ to be
broken in the tree, waiting for a possible future enhancement that no
one has committed to writing. But it's your call -- please apply either
this patch, or the previous one (-3) as you see fit.

Cheers,

Neil

--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC

I think it is still a bad idea to leave code that is _known_ to be
broken in the tree, waiting for a possible future enhancement that no
one has committed to writing. But it's your call -- please apply either
this patch, or the previous one (-3) as you see fit.

We have been bitten by refusing patches that have a valid purpose,
supposing someone will come and something that may take years to do,
e.g. nocreatable permissions. If it does something useful, and can be
ripped out later, why not add it?

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

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

We have been bitten by refusing patches that have a valid purpose,
supposing someone will come and something that may take years to do,
e.g. nocreatable permissions. If it does something useful, and can be
ripped out later, why not add it?

But it doesn't do anything useful, AFAICS. What significant problem
occurs if an index shows an owner different from its parent? If there's
any actual operational problem there, then I'd agree we need a fix.

regards, tom lane

Tom Lane wrote:

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

We have been bitten by refusing patches that have a valid purpose,
supposing someone will come and something that may take years to do,
e.g. nocreatable permissions. If it does something useful, and can be
ripped out later, why not add it?

But it doesn't do anything useful, AFAICS. What significant problem
occurs if an index shows an owner different from its parent? If there's
any actual operational problem there, then I'd agree we need a fix.

Oh, I see.

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

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

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

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

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

Neil Conway wrote:

On Mon, 2002-02-25 at 18:48, Tom Lane wrote:

Neil Conway <nconway@klamath.dyndns.org> writes:

Additionally, if someone eventually fixes the index-ownership situation,
the changes to command.c to remove the index recursion are trivial.

... but won't necessarily get done. More to the point, they may confuse
someone who's trying to refactor the code: without careful thought, he
might think he needs to support recursion over indexes as well as child
tables.

Not if the code includes a comment (as it does) that the recursion is
intended _only_ to support changing the ownership of any indexes which
belong to the table.

IMHO, it's not confusing at all: in the current code, indexes have
owners, and should be owned by the owner of the table they belong to.
The patch makes this consistent; without the patch, one might conclude
that there are reasonable situations in the owner of a table should not
own its indexes, which is incorrect AFAIK.

BTW, should ownership be removed from sequences as well?

This patch also includes some refactoring and code cleanups that are
useful in any case.

Sure. Please resubmit just that part.

Okay, I've attached a patch which implements this.

I think it is still a bad idea to leave code that is _known_ to be
broken in the tree, waiting for a possible future enhancement that no
one has committed to writing. But it's your call -- please apply either
this patch, or the previous one (-3) as you see fit.

Cheers,

Neil

--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC

[ Attachment, skipping... ]

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

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

OK, the issue with this patch is that it fixes ownership of INDEXES.
The complaint is that we shouldn't have ownership of indexes. However,
we are clearly now preventing anyone except the table owner from
creating indexes, so it seems there is ownership, or at least a
restriction.

Now, we are we going with this? Can we just remove ownership of indexes
totally? And sequences? Is that the direction we want to go in? I
can't imagine it is very hard to do. Or is the problem that we should
not be reporting the owner of these items?

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

Neil Conway wrote:

On Mon, 2002-02-25 at 18:48, Tom Lane wrote:

Neil Conway <nconway@klamath.dyndns.org> writes:

Additionally, if someone eventually fixes the index-ownership situation,
the changes to command.c to remove the index recursion are trivial.

... but won't necessarily get done. More to the point, they may confuse
someone who's trying to refactor the code: without careful thought, he
might think he needs to support recursion over indexes as well as child
tables.

Not if the code includes a comment (as it does) that the recursion is
intended _only_ to support changing the ownership of any indexes which
belong to the table.

IMHO, it's not confusing at all: in the current code, indexes have
owners, and should be owned by the owner of the table they belong to.
The patch makes this consistent; without the patch, one might conclude
that there are reasonable situations in the owner of a table should not
own its indexes, which is incorrect AFAIK.

BTW, should ownership be removed from sequences as well?

This patch also includes some refactoring and code cleanups that are
useful in any case.

Sure. Please resubmit just that part.

Okay, I've attached a patch which implements this.

I think it is still a bad idea to leave code that is _known_ to be
broken in the tree, waiting for a possible future enhancement that no
one has committed to writing. But it's your call -- please apply either
this patch, or the previous one (-3) as you see fit.

Cheers,

Neil

--
Neil Conway <neilconway@rogers.com>
PGP Key ID: DB3C29FC

[ Attachment, skipping... ]

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

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

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

OK, the issue with this patch is that it fixes ownership of INDEXES.

I thought the resubmitted patch did no such thing?

Now, we are we going with this? Can we just remove ownership of indexes
totally? And sequences?

How did you get from indexes to sequences? The issues are completely
different.

I'm in favor of considering that indexes and toast tables have no
separate ownership, and storing zero in pg_class.relowner for them.
However, I have not looked to see what this might break. It might
be more trouble than it's worth.

regards, tom lane