Re: [v9.2] Fix leaky-view problem, part 1

On Fri, Jul 08, 2011 at 09:20:46AM +0100, Kohei KaiGai wrote:
> 2011/7/7 Noah Misch <noah(at)2ndquadrant(dot)com>:
> > On Thu, Jul 07, 2011 at 03:56:26PM +0100, Kohei KaiGai wrote:
> >> 2011/7/7 Noah Misch <noah(at)2ndquadrant(dot)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