Re: Rationalizing Query.withCheckOptions

Tom,

* Tom Lane (tgl(at)sss(dot)pgh(dot)pa(dot)us) wrote:
> The RLS patch added this to struct Query:
>
> List *withCheckOptions; /* a list of WithCheckOption's, which
> * are only added during rewrite and
> * therefore are not written out as
> * part of Query. */
>
> As per the comment, this field is ignored by outfuncs.c and readfuncs.c.
> That's pretty horrid, because:
>
> 1. Omitting the field from the output of pprint() seriously impedes
> debugging. I could probably have wasted significantly fewer hours
> yesterday before figuring out fd195257 if pprint() hadn't been lying
> to me about what was in the query data structures.

It used to be. It was removed in 4158cc3, per our discussion about
that being essentially dead code which wouldn't be exercised as it'll
always be NIL.

> 2. At some point, this will break parallel queries, or perhaps just
> cause them to silently fail to enforce RLS, because we depend on
> outfuncs/readfuncs to transfer node trees to parallel workers.
> (I think there's no live bug today because we only transfer Plans
> not Queries, but surely this is a loaded foot-gun.)

I saw Robert's commit and had a similar concern and similar conclusion
that I don't believe it's an issue today.

> 3. A node type as fundamental as Query ought not have weird gotchas
> like this.

> Furthermore, as far as I can see there's actually no point at all to the
> special exception. As the comment says, the list does not get populated
> until rewrite time, which means that if outfuncs/readfuncs just process it
> normally, it would always be NIL in stored views anyway.

I don't have any problem with it being included as long as everyone's
fine with it ending up on disk as an always-NIL value.

> It's too late to change this in 9.5, but I think we should do so
> in HEAD.

I'd be happy to revert those bits of 4158cc37 and update the comment
accordingly.

Thanks!

Stephen