Re: Add support for restrictive RLS policies

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.

Thanks!

Stephen