From: | Kohei KaiGai <kaigai(at)kaigai(dot)gr(dot)jp> |
---|---|
To: | Noah Misch <noah(at)2ndquadrant(dot)com>, Robert Haas <robertmhaas(at)gmail(dot)com> |
Cc: | 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 14:41:41 |
Message-ID: | CADyhKSX25qLXB+yDKqe_J=ao7LUHPYsxHYUVvxtnQdZZ0bsQcQ@mail.gmail.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
The attached patch is a revised one; that utilizes untransformRelOptions()
to construct a list of DefElem to be supplied into AT_ResetRelOptions
commands. It enabled me to implement more compact as I expected.
How about this approach to reset existing reloptions?
I'll consolidate part-0, 1 and 2 patches after we make fix the direction to
distinguish leaky qualifiers from others, in the thread of part-2.
Right now, I'm considering the right way to choose qualifiers to be
transformed into index scans.
Thanks,
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.
>
>> 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
>
--
KaiGai Kohei <kaigai(at)kaigai(dot)gr(dot)jp>
Attachment | Content-Type | Size |
---|---|---|
pgsql-v9.2-fix-leaky-view-part-0.v5.patch | text/x-patch | 23.2 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Florian Pflug | 2011-07-08 14:43:46 | Re: spinlock contention |
Previous Message | Heikki Linnakangas | 2011-07-08 14:34:24 | Re: Re: [COMMITTERS] pgsql: Adjust OLDSERXID_MAX_PAGE based on BLCKSZ. |