Re: Add support for restrictive RLS policies

On Mon, Sep 12, 2016 at 7:27 AM, Stephen Frost <sfrost(at)snowman(dot)net> wrote:

> Robert,
>
> * 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.

Thanks

>
> Thanks!
>
> Stephen
>

--
Jeevan B Chalke
Principal Software Engineer, Product Development
EnterpriseDB Corporation
The Enterprise PostgreSQL Company