Thread: Rename withCheckOptions to insertedCheckClauses

All,

and this totally should have gone to -hackers instead, sorry about that.
Please ignore the one to -committers, if possible.

Tom, all,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> My vote would be to rename and reposition the field in HEAD and 9.5
> and accept the corresponding initdb.  We already forced an initdb
> since alpha2, so it's basically free as far as testers are concerned,
> and keeping 9.5 in sync with HEAD in this area seems like a really
> good idea for awhile to come.

Alright, attached is an attempt at doing these renames.  I went a bit
farther since it seemed to make sense to me (at least at the time, I'm
wondering a bit about it now), so, please provide any thoughts or
feedback you have regarding these changes.

Thanks!

Stephen

Stephen Frost <sfrost@snowman.net> writes:
> Alright, attached is an attempt at doing these renames.  I went a bit
> farther since it seemed to make sense to me (at least at the time, I'm
> wondering a bit about it now), so, please provide any thoughts or
> feedback you have regarding these changes.

> Further, rename 'withCheckOptions' in Query and ModifyTable to
> 'insertedCheckClauses', ExecWithCheckOption to ExecCheckClauses,
> ri_WithCheckOptions and ri_WithCheckOptionExprs to
> ri_InsertedCheckClauses and ri_InsertedCheckExprs, respectively, all to
> make it clear that these are the check clauses which were added by the
> rewriter, not the actual WITH CHECK OPTION indication for a view (which
> is stored in reloptions for the view) nor the WITH CHECK expressions for

That commitlog entry doesn't seem to quite square with the patch,
wherein I see

- *        WithCheckOptions        list of WithCheckOption's to be checked
- *        WithCheckOptionExprs    list of WithCheckOption expr states
+ *        InsertedCheckClauses    list of WithCheckOption's to be checked
+ *        InsertedCheckClauseExprs    list of WithCheckOption expr states

-    List       *ri_WithCheckOptions;
-    List       *ri_WithCheckOptionExprs;
+    List       *ri_InsertedCheckClauses;
+    List       *ri_InsertedCheckClauseExprs;

The distinction between a "clause" and an "expr" is not very obvious,
and certainly most other places in the code use those terms pretty
interchangeably, so I find both the old and new names unclear here.
How about ri_InsertedCheckClauseStates instead for the second list?
And similarly if you're using "Expr" to mean ExprState anywhere else.
        regards, tom lane

I wrote:
> -    List       *ri_WithCheckOptions;
> -    List       *ri_WithCheckOptionExprs;
> +    List       *ri_InsertedCheckClauses;
> +    List       *ri_InsertedCheckClauseExprs;

> The distinction between a "clause" and an "expr" is not very obvious,
> and certainly most other places in the code use those terms pretty
> interchangeably, so I find both the old and new names unclear here.
> How about ri_InsertedCheckClauseStates instead for the second list?
> And similarly if you're using "Expr" to mean ExprState anywhere else.

Actually ... does struct ResultRelInfo need to carry the original WCO
clauses at all, rather than just the exprstate list?  In most places
we do not store expr and exprstate lists in the same node in the first
place, so we can get away with using the same field name for corresponding
lists in plan and planstate nodes.  That's why we don't already have a
convention like "fooStates" for such lists.

Another thought is that as long as these are lists specifically of
WithCheckOption nodes, and not arbitrary expressions, "clause" isn't an
especially good term for them; it implies generality that isn't there.
And CheckClauses invites confusion with, for example, CHECK clauses of
domain types.  So maybe better names would be "ri_InsertedCheckOptions"
(and "ri_InsertedCheckOptionStates" if you still need that).  Or maybe
"ri_InsertedWCOClauses" and "ri_InsertedWCOClauseStates".  I'm less sure
about whether this is an improvement, though.
        regards, tom lane

On 24 September 2015 at 21:25, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
>> -     List       *ri_WithCheckOptions;
>> -     List       *ri_WithCheckOptionExprs;
>> +     List       *ri_InsertedCheckClauses;
>> +     List       *ri_InsertedCheckClauseExprs;
>
>> The distinction between a "clause" and an "expr" is not very obvious,
>> and certainly most other places in the code use those terms pretty
>> interchangeably, so I find both the old and new names unclear here.
>> How about ri_InsertedCheckClauseStates instead for the second list?
>> And similarly if you're using "Expr" to mean ExprState anywhere else.
>
> Actually ... does struct ResultRelInfo need to carry the original WCO
> clauses at all, rather than just the exprstate list?  In most places
> we do not store expr and exprstate lists in the same node in the first
> place, so we can get away with using the same field name for corresponding
> lists in plan and planstate nodes.  That's why we don't already have a
> convention like "fooStates" for such lists.
>
> Another thought is that as long as these are lists specifically of
> WithCheckOption nodes, and not arbitrary expressions, "clause" isn't an
> especially good term for them; it implies generality that isn't there.
> And CheckClauses invites confusion with, for example, CHECK clauses of
> domain types.  So maybe better names would be "ri_InsertedCheckOptions"
> (and "ri_InsertedCheckOptionStates" if you still need that).  Or maybe
> "ri_InsertedWCOClauses" and "ri_InsertedWCOClauseStates".  I'm less sure
> about whether this is an improvement, though.
>

None of this renaming seems like an improvement to me.

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".

ri_InsertedCheckOptionStates is inconsistent with the names of the
other lists of expr states on that node -- ri_TrigWhenExprs and
ri_ConstraintExprs.

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

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.

Regards,
Dean

* Dean Rasheed (dean.a.rasheed@gmail.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

Stephen Frost <sfrost@snowman.net> writes:
> * Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
>> 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.

Yes.  I do not think that we should stick with badly chosen names just
because of back-patching concerns.  By that argument, we should never
fix any erroneous comments either.

Whether these particular names are improvements is, of course, a fit
subject for debate.  I have to agree that I don't feel like we've quite
hit on le mot juste yet.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Stephen Frost <sfrost@snowman.net> writes:
> > * Dean Rasheed (dean.a.rasheed@gmail.com) wrote:
> >> 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.
>
> Yes.  I do not think that we should stick with badly chosen names just
> because of back-patching concerns.  By that argument, we should never
> fix any erroneous comments either.
>
> Whether these particular names are improvements is, of course, a fit
> subject for debate.  I have to agree that I don't feel like we've quite
> hit on le mot juste yet.

I've gone ahead and at least removed the withCheckOptions empty-list
from being written out as part of Query for 9.5 and HEAD, and bumped
catversion accordingly.

I came to realize that ModifyTable actually is planned to be used for
parallel query and therefore the list for that needs to stay, along with
support for reading the WithCheckOption node in.

Thanks!

Stephen