Re: Rename withCheckOptions to insertedCheckClauses

* Dean Rasheed (dean(dot)a(dot)rasheed(at)gmail(dot)com) wrote:
> None of this renaming seems like an improvement to me.

I have to admit that I'm not entirely sure it's improving things either.

Certainly, we shouldn't be dumping out the withCheckOptions node and
perhaps we should move its place in Query and add comments to it, but
I'm not sure about the other changes.

> ri_InsertedCheckClauses is misleading because they're not clauses,
> they're WithCheckOption nodes carrying various pieces of information,
> only one of which is the clause to check.
>
> ri_InsertedCheckOptions is a bit better from that perspective, but it
> still seems messy for the executor to carry the knowledge that these
> checks were inserted by the rewriter. In the executor they're just
> checks to be executed, and "WithCheck" reflects their original source
> better than "InsertedCheck".

Yeah, the actual structure is 'WithCheckOption' and everything is pretty
consistent around that. One possibly confusing bit is that we have a
ViewCheckOption enum in ViewStmt called "withCheckOption". Perhaps
that's what we should change? Semes like it's either that or rename the
structure itself to NewTupleCheck (or similar) and rename everything to
go along with that instead.

> Also, these were added in 9.4, so introducing this many differences
> between 9.4 and 9.5+ will make back-patching harder.

That's certainly true, but we don't want current or future hackers to be
confused either.

> The original objection to the name WithCheckOptions was in relation to
> the Query struct, and the fact that this field isn't the parsed
> representation of WITH CHECK OPTION's on the query. I think that can
> be cured with a suitable comment.

There's also the issue that outfuncs is including that node when it
really shouldn't be. That can be fixed independnetly of this
discussion, of course.

Thanks!

Stephen