On Fri, Jul 08, 2011 at 09:20:46AM +0100, Kohei KaiGai wrote: > 2011/7/7 Noah Misch <n...@2ndquadrant.com>: > > On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote: > >> 2011/7/7 Noah Misch <n...@2ndquadrant.com>: > >> > On Wed, Jul 06, 2011 at 10:25:12PM +0200, Kohei KaiGai wrote:
> >> > That gets the job done for today, but DefineVirtualRelation() should not > >> > need > >> > to know all view options by name to simply replace the existing list > >> > with a > >> > new one. ?I don't think you can cleanly use the ALTER TABLE SET/RESET > >> > code for > >> > this. ?Instead, compute an option list similar to how DefineRelation() > >> > does so > >> > at tablecmds.c:491, then update pg_class. > >> > > >> My opinion is ALTER TABLE SET/RESET code should be enhanced to accept > >> an operation to reset all the existing options, rather than tricky > >> updates of pg_class. > > > > The pg_class update has ~20 lines of idiomatic code; see > > tablecmds.c:7931-7951. > > > Even if idiomatic, another part of DefineVirtualRelation() uses > AlterTableInternal(). > I think a common way is more straightforward. The fact that we use ALTER TABLE ADD COLUMN in DefineVirtualRelation() is not itself cause to use ALTER TABLE SET (...) nearby. We should do so only if it brings some advantage, like simpler or more-robust code. I'm not seeing either advantage. Those can be points of style, so perhaps I have the poor taste here. > So, how about an idea to add a function that pull-out existing options > from syscache, > and merge with the supplied options list prior to AlterTableInternal()? It seems wrong to me to trawl through the view's existing option list just to replace it completely with a new option list. Again, it's subjective. If you'd like to proceed with this and let the committer decide, that's fine with me. Thanks, nm -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
