Re: Add support for restrictive RLS policies

From: Stephen Frost <sfrost(at)snowman(dot)net>
To: Jeevan Chalke <jeevan(dot)chalke(at)enterprisedb(dot)com>
Cc: Robert Haas <robertmhaas(at)gmail(dot)com>, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>, Alvaro Herrera <alvherre(at)2ndquadrant(dot)com>, Thom Brown <thom(at)linux(dot)com>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>
Subject: Re: Add support for restrictive RLS policies
Date: 2016-09-26 19:37:25
Message-ID: 20160926193725.GJ5148@tamriel.snowman.net
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Jeevan, all,

* Jeevan Chalke (jeevan(dot)chalke(at)enterprisedb(dot)com) wrote:
> On Mon, Sep 12, 2016 at 7:27 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:
> > * Robert Haas (robertmhaas(at)gmail(dot)com) wrote:
> > > On Thu, Sep 8, 2016 at 5:21 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > > > Stephen Frost <sfrost(at)snowman(dot)net> writes:
> > > >> * Alvaro Herrera (alvherre(at)2ndquadrant(dot)com) wrote:
> > > >>> Can't you keep those words as Sconst or something (DefElems?) until
> > the
> > > >>> execution phase, so that they don't need to be keywords at all?
> > > >
> > > >> Seems like we could do that, though I'm not convinced that it really
> > > >> gains us all that much. These are only unreserved keywords, of
> > course,
> > > >> so they don't impact users the way reserved keywords (of any kind)
> > can.
> > > >> While there may be some places where we use a string to represent a
> > set
> > > >> of defined options, I don't believe that's typical
> > > >
> > > > -1 for having to write them as string literals; but I think what Alvaro
> > > > really means is to arrange for the words to just be identifiers in the
> > > > grammar, which you strcmp against at execution. See for example
> > > > reloption_list. (Whether you use DefElem as the internal
> > representation
> > > > is a minor detail, though it might help for making the parsetree
> > > > copyObject-friendly.)
> > > >
> > > > vacuum_option_elem shows another way to avoid making a word into a
> > > > keyword, although to me that one is more of an antipattern; it'd be
> > better
> > > > to leave the strcmp to execution, since there's so much other code that
> > > > does things that way.
> > >
> > > There are other cases like that, too, e.g. AlterOptRoleElem; I don't
> > > think it's a bad pattern. Regardless of the specifics, I do think
> > > that it would be better not to bloat the keyword table with things
> > > that don't really need to be keywords.
> >
> > The AlterOptRoleElem case is, I believe, what Tom was just suggesting as
> > an antipattern, since the strcmp() is being done at parse time instead
> > of at execution time.
> >
> > If we are concerned about having too many unreserved keywords, then I
> > agree that AlterOptRoleElem is a good candidate to look at for reducing
> > the number we have, as it appears to contain 3 keywords which are not
> > used anywhere else (and 1 other which is only used in one other place).
> >
> > I do think that using IDENT for the various role attributes does make
> > sense, to be clear, as there are quite a few of them, they change
> > depending on major version, and those keywords are very unlikely to ever
> > have utilization elsewhere.
> >
> > For this case, there's just 2 keywords which seem like they may be used
> > again (perhaps for ALTER or DROP POLICY, or default policies if we grow
> > those in the future), and we're unlikly to grow more in the future for
> > that particular case (as we only have two binary boolean operators and
> > that seems unlikely to change), though, should that happens, we could
> > certainly revisit this.
> >
>
> To me, adding two new keywords for two new options does not look good as it
> will bloat keywords list. Per my understanding we should add keyword if and
> only if we have no option than adding at, may be to avoid grammar conflicts.
>
> I am also inclined to think that using identifier will be a good choice
> here.
> However I would prefer to do the string comparison right into the grammar
> itself, so that if we have wrong option as input there, then we will not
> proceed further with it. We are anyway going to throw an error later then
> why not at early stage.

Updated patch changes to using IDENT and an strcmp() (similar to
AlterOptRoleElem and vacuum_option_elem) to check the results at parse-time,
and then throwing a more specific error if an unexpected value is found
(instead of just 'syntax error'). This avoids adding any keywords.

Thanks!

Stephen

Attachment Content-Type Size
restrict_rls_v3.patch text/x-diff 50.2 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Tom Lane 2016-09-26 19:38:03 Re: [GENERAL] inconsistent behaviour of set-returning functions in sub-query with random()
Previous Message Stephen Frost 2016-09-26 19:27:33 Re: Add support for restrictive RLS policies