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

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:
> >> *** a/src/backend/commands/view.c
> >> --- b/src/backend/commands/view.c
> >
> >> --- 227,257 ----
> >>                               atcmd->def = (Node *) lfirst(c);
> >>                               atcmds = lappend(atcmds, atcmd);
> >>                       }
> >>               }
> >>
> >>               /*
> >> +              * If optional parameters are specified, we must set options
> >> +              * using ALTER TABLE SET OPTION internally.
> >> +              */
> >> +             if (list_length(options) > 0)
> >> +             {
> >> +                     atcmd = makeNode(AlterTableCmd);
> >> +                     atcmd->subtype = AT_SetRelOptions;
> >> +                     atcmd->def = (List *)options;
> >> +
> >> +                     atcmds = lappend(atcmds, atcmd);
> >> +             }
> >> +             else
> >> +             {
> >> +                     atcmd = makeNode(AlterTableCmd);
> >> +                     atcmd->subtype = AT_ResetRelOptions;
> >> +                     atcmd->def = (Node *) list_make1(makeDefElem("security_barrier",
> >> +                                                                                                              NULL));
> >> +             }
> >> +             if (atcmds != NIL)
> >> +                     AlterTableInternal(viewOid, atcmds, true);
> >> +
> >> +             /*
> >>                * Seems okay, so return the OID of the pre-existing view.
> >>                */
> >>               relation_close(rel, NoLock);    /* keep the lock! */
> >
> > 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.

> How about an idea to add AT_ResetAllRelOptions for internal use only?

If some operation is purely internal and does not otherwise benefit from the
ALTER TABLE infrastructure, there's no benefit in involving ALTER TABLE.
DefineVirtualRelation() uses ALTER TABLE to add columns because all that code
needs to exist anyway. You could make a plain function to do the update that
gets called from both ATExecSetRelOptions() and DefineVirtualRelation().

Thanks,
nm