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

2011/7/8 Noah Misch <noah(at)2ndquadrant(dot)com>:
> 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.
>
The attached patch is a revised version according to the approach that updates
pg_class system catalog before AlterTableInternal().
It invokes the new ResetViewOptions when rel->rd_options is not null, and it set
null on the pg_class.reloptions of the view and increments command counter.

Rest of stuffs are not changed from the v5.

Thanks,
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>