On 24 September 2015 at 21:25, Tom Lane <t...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
