Re: [HACKERS] Rename withCheckOptions to insertedCheckClauses
* Tom Lane (t...@sss.pgh.pa.us) wrote: > Stephen Frost writes: > > * Dean Rasheed (dean.a.rash...@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 signature.asc Description: Digital signature
Re: [HACKERS] Rename withCheckOptions to insertedCheckClauses
Stephen Frost writes: > * Dean Rasheed (dean.a.rash...@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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Rename withCheckOptions to insertedCheckClauses
* Dean Rasheed (dean.a.rash...@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 signature.asc Description: Digital signature
Re: [HACKERS] Rename withCheckOptions to insertedCheckClauses
On 24 September 2015 at 21:25, Tom Lane 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
Re: [HACKERS] Rename withCheckOptions to insertedCheckClauses
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Rename withCheckOptions to insertedCheckClauses
Stephen Frost 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 - * WithCheckOptionslist of WithCheckOption's to be checked - * WithCheckOptionExprslist of WithCheckOption expr states + * InsertedCheckClauseslist of WithCheckOption's to be checked + * InsertedCheckClauseExprslist 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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers