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

From: Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp>
To: Noah Misch <noah(at)2ndquadrant(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Kohei Kaigai <Kohei(dot)Kaigai(at)emea(dot)nec(dot)com>, Stephen Frost <sfrost(at)snowman(dot)net>, Heikki Linnakangas <heikki(dot)linnakangas(at)enterprisedb(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: [v9.2] Fix leaky-view problem, part 1
Date: 2011-07-08 08:20:46
Message-ID: CADyhKSUbSsn2Rpb4Rqu=sSbe=wFgRxfcHdt6QfkPr=fh5cSePg@mail.gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

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:
>> >> *** 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.
>
Even if idiomatic, another part of DefineVirtualRelation() uses
AlterTableInternal().
I think a common way is more straightforward.

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()?

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

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Kohei KaiGai 2011-07-08 09:09:54 Re: [v9.2] Fix leaky-view problem, part 2
Previous Message Heikki Linnakangas 2011-07-08 08:18:19 Re: [v9.2] Fix leaky-view problem, part 2