Re: [HACKERS] nodes/*funcs.c inconsistencies
On Mon, Aug 3, 2015 at 10:00 AM, Alvaro Herrera wrote: >> Couldn't we adopt >> AssertVariableIsOfType()/AssertVariableIsOfTypeMacro() to macros like >> READ_UINT_FIELD()? >> >> I'm surprised that this stuff was only ever used for logical decoding >> infrastructure so far. > > The reason it's only used there is that Andres is the one who introduced > those macros precisely for that code. We've not yet had time to adjust > the rest of the code to have more sanity checks. Any chance of picking this up, Andres? -- Peter Geoghegan -- 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] nodes/*funcs.c inconsistencies
Noah, * Noah Misch (n...@leadboat.com) wrote: > Rather than commit on an emergency basis in the few hours between these +1's > and the wrap, I kept to my original schedule. FYI, if it hadn't required > emergency procedures (cancelling the day's plans so I could get to a notebook > computer), I would have committed early based on the three +1s. No worries, fully agreed that this was not an emergency in any way. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] nodes/*funcs.c inconsistencies
On Mon, Aug 03, 2015 at 09:57:52AM -0700, Joe Conway wrote: > On 08/03/2015 09:55 AM, Stephen Frost wrote: > > * Noah Misch (n...@leadboat.com) wrote: > >> On Sun, Aug 02, 2015 at 11:31:16PM -0400, Tom Lane wrote: > >>> That being the case, it would probably be a good idea to get > >>> them done before alpha2, as there may not be a good opportunity > >>> afterwards. > >> > >> Freedom to bump catversion after alpha2 will be > >> barely-distinguishable from freedom to do so now. I have planned > >> to leave my usual comment period of a few days, though skipping > >> that would be rather innocuous in this case. > > > > This is clearly necessary, of course, and I wouldn't be surprised > > if we have another necessary bump post-alpha2, but at the same > > time, it'd certainly be nice if we are able to put out alpha2 and > > then a beta1 in the future without needing to do a bump. > > > > In other words, +1 from me for going ahead and committing this for > > alpha2, if possible. > > +1 Rather than commit on an emergency basis in the few hours between these +1's and the wrap, I kept to my original schedule. FYI, if it hadn't required emergency procedures (cancelling the day's plans so I could get to a notebook computer), I would have committed early based on the three +1s. -- 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] nodes/*funcs.c inconsistencies
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 08/03/2015 09:55 AM, Stephen Frost wrote: > * Noah Misch (n...@leadboat.com) wrote: >> On Sun, Aug 02, 2015 at 11:31:16PM -0400, Tom Lane wrote: >>> That being the case, it would probably be a good idea to get >>> them done before alpha2, as there may not be a good opportunity >>> afterwards. >> >> Freedom to bump catversion after alpha2 will be >> barely-distinguishable from freedom to do so now. I have planned >> to leave my usual comment period of a few days, though skipping >> that would be rather innocuous in this case. > > This is clearly necessary, of course, and I wouldn't be surprised > if we have another necessary bump post-alpha2, but at the same > time, it'd certainly be nice if we are able to put out alpha2 and > then a beta1 in the future without needing to do a bump. > > In other words, +1 from me for going ahead and committing this for > alpha2, if possible. +1 - -- Joe Conway -BEGIN PGP SIGNATURE- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJVv52QAAoJEDfy90M199hl/skP+wXfyo8axMipFwyV9fqesFHF xRc8j2QHUOVTyqAA9TdksSL1+t8h7a6PmEL6ucIUyGIfcBZaY5kw9/IOQxac8i0W ZxAElSzFSL8xpimSehZCKLHTF4fTlypSA1srVOb2py6vjOQK+7PZAnlP42TXEgBT gk6HnAFQBJ6cx8KckYHdqQFPtTKLyglSFLeOCBfbwymOBZgjEopdzTDsE17Z07m8 ZQq08jiSPJKGmwiLI7SCMlA5DMI6B9OV2/h8GXfxP833oR8Z8wQrvzQkM0GnJU5w Hirk8ECEJndBuLyyVl0suzabNUNdzVWvXQHMIP2+0aW/secAiUbkVdF7o0B5l8qI GZaHFCZ37BdKgppjApCCLmA/1Th2hHKEwGsIWGiBMQhWfKeRzXxuUZYqSF+YZDRN ju5ZhJOlmKiLv0AyuMPw9T25X8fTzBwAppjL7r6HM610rNOM0izYz8+7K0ckq74/ jfbt+FuKxGwp8Phz7mm7APOsDsrhNkQgmn1WpnqgJBQwXTRZGzn2ne9npvmUAo3C 4Hiaib2fTIq2Q39scT51b5enjtkPa9S2o4MF+32JP6g5UmLMe3GMpa5V2itvCuN3 ue8AzgNoeyd3oOlqPh9XJi/Kk68it4UacXRkiZAJ9cLTEMixVUjuaVkCyFxsmKxb vvNa2Of440JgSc2Hw21p =vdZZ -END PGP SIGNATURE- -- 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] nodes/*funcs.c inconsistencies
Peter Geoghegan wrote: > Couldn't we adopt > AssertVariableIsOfType()/AssertVariableIsOfTypeMacro() to macros like > READ_UINT_FIELD()? > > I'm surprised that this stuff was only ever used for logical decoding > infrastructure so far. The reason it's only used there is that Andres is the one who introduced those macros precisely for that code. We've not yet had time to adjust the rest of the code to have more sanity checks. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] nodes/*funcs.c inconsistencies
Peter Geoghegan wrote: > On Sun, Aug 2, 2015 at 3:07 PM, Peter Geoghegan wrote: > > I'm surprised that this stuff was only ever used for logical decoding > > infrastructure so far. > > On second thought, having tried it, one reason is that that breaks > things that are considered legitimate for historical reasons. For > example, AttrNumber is often used with READ_INT_FIELD(), which is an > int16. Whether or not it's worth fixing that by introducing a > READ_ATTRNUM_FIELD() (and so on) is not obvious to me. If it allows us to introduce additional checking for new code, I'm all for it. -- Álvaro Herrerahttp://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- 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] nodes/*funcs.c inconsistencies
* Noah Misch (n...@leadboat.com) wrote: > On Sun, Aug 02, 2015 at 11:31:16PM -0400, Tom Lane wrote: > > That being the case, it would probably be a good idea to get them done > > before alpha2, as there may not be a good opportunity afterwards. > > Freedom to bump catversion after alpha2 will be barely-distinguishable from > freedom to do so now. I have planned to leave my usual comment period of a > few days, though skipping that would be rather innocuous in this case. This is clearly necessary, of course, and I wouldn't be surprised if we have another necessary bump post-alpha2, but at the same time, it'd certainly be nice if we are able to put out alpha2 and then a beta1 in the future without needing to do a bump. In other words, +1 from me for going ahead and committing this for alpha2, if possible. Thanks! Stephen signature.asc Description: Digital signature
Re: [HACKERS] nodes/*funcs.c inconsistencies
On Sun, Aug 02, 2015 at 11:31:16PM -0400, Tom Lane wrote: > Stephen Frost writes: > > Noah, > >> A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic > >> INT/UINT or field order corrections. The non-cosmetic changes involve > >> CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the > >> existing treatments (ignore custom_plans/custom_paths fields; copy/compare > >> "cmd" string pointer as a scalar) were deliberate, please let me know. > > > Thanks for the review. The change you have is correct for > > CreatePolicyStmt, at least. I imagine I confused it with polcmd, which > > is actually just a char. > > > Barring objections, I'll change it to cmd_name after your commit, to > > reduce the chances of future confusion. The existing identifier seems fine, but won't I mind that change, either. > Both of you please keep in mind that these "cosmetic" changes are > initdb-forcing, at least if they affect node types that can appear > in stored rules. Right; Stephen's does not force initdb, but some of what I posted does so. > That being the case, it would probably be a good idea to get them done > before alpha2, as there may not be a good opportunity afterwards. Freedom to bump catversion after alpha2 will be barely-distinguishable from freedom to do so now. I have planned to leave my usual comment period of a few days, though skipping that would be rather innocuous in this case. -- 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] nodes/*funcs.c inconsistencies
Stephen Frost writes: > Noah, >> A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic >> INT/UINT or field order corrections. The non-cosmetic changes involve >> CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the >> existing treatments (ignore custom_plans/custom_paths fields; copy/compare >> "cmd" string pointer as a scalar) were deliberate, please let me know. > Thanks for the review. The change you have is correct for > CreatePolicyStmt, at least. I imagine I confused it with polcmd, which > is actually just a char. > Barring objections, I'll change it to cmd_name after your commit, to > reduce the chances of future confusion. Both of you please keep in mind that these "cosmetic" changes are initdb-forcing, at least if they affect node types that can appear in stored rules. That being the case, it would probably be a good idea to get them done before alpha2, as there may not be a good opportunity afterwards. 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] nodes/*funcs.c inconsistencies
On Mon, Aug 03, 2015 at 01:32:10AM +, Kouhei Kaigai wrote: > > On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote: > > > I observed these inconsistencies in node support functions: > > > > A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic > > INT/UINT or field order corrections. The non-cosmetic changes involve > > CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the > > existing treatments (ignore custom_plans/custom_paths fields; copy/compare > > "cmd" string pointer as a scalar) were deliberate, please let me know. > > > Thanks for your works. > > I also noticed one other inconsistent point; _outMergeJoin() dumps > mergeNullsFirst[] array but it does not use booltostr() macro. Good catch. All supported branches work that way, and it's not wrong. I recommend keeping it unchanged. -- 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] nodes/*funcs.c inconsistencies
> On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote: > > I observed these inconsistencies in node support functions: > > A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic > INT/UINT or field order corrections. The non-cosmetic changes involve > CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the > existing treatments (ignore custom_plans/custom_paths fields; copy/compare > "cmd" string pointer as a scalar) were deliberate, please let me know. > Thanks for your works. I also noticed one other inconsistent point; _outMergeJoin() dumps mergeNullsFirst[] array but it does not use booltostr() macro. Best regards, -- NEC Business Creation Division / PG-Strom Project KaiGai Kohei -- 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] nodes/*funcs.c inconsistencies
Noah, * Noah Misch (n...@leadboat.com) wrote: > On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote: > > I observed these inconsistencies in node support functions: > > A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic > INT/UINT or field order corrections. The non-cosmetic changes involve > CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the > existing treatments (ignore custom_plans/custom_paths fields; copy/compare > "cmd" string pointer as a scalar) were deliberate, please let me know. Thanks for the review. The change you have is correct for CreatePolicyStmt, at least. I imagine I confused it with polcmd, which is actually just a char. Barring objections, I'll change it to cmd_name after your commit, to reduce the chances of future confusion. Thanks again! Stephen signature.asc Description: Digital signature
Re: [HACKERS] nodes/*funcs.c inconsistencies
On Sun, Aug 2, 2015 at 3:07 PM, Peter Geoghegan wrote: > I'm surprised that this stuff was only ever used for logical decoding > infrastructure so far. On second thought, having tried it, one reason is that that breaks things that are considered legitimate for historical reasons. For example, AttrNumber is often used with READ_INT_FIELD(), which is an int16. Whether or not it's worth fixing that by introducing a READ_ATTRNUM_FIELD() (and so on) is not obvious to me. -- Peter Geoghegan -- 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] nodes/*funcs.c inconsistencies
On Sun, Aug 2, 2015 at 1:28 PM, Noah Misch wrote: > A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic > INT/UINT or field order corrections. I was responsible for a couple of the cosmetic ones. Sorry about that. It occurs to me that we could do a little more to prevent this automatically. Couldn't we adopt AssertVariableIsOfType()/AssertVariableIsOfTypeMacro() to macros like READ_UINT_FIELD()? I'm surprised that this stuff was only ever used for logical decoding infrastructure so far. -- Peter Geoghegan -- 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] nodes/*funcs.c inconsistencies
Noah Misch writes: > On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote: >> I observed these inconsistencies in node support functions: > A fresh audit found the attached problems new in 9.5[1]. Many thanks for doing that; I'd had the same checking on my personal to-do list, but now will not need to. 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] nodes/*funcs.c inconsistencies
On Mon, Apr 16, 2012 at 06:25:15AM -0400, Noah Misch wrote:
> I observed these inconsistencies in node support functions:
A fresh audit found the attached problems new in 9.5[1]. Most are cosmetic
INT/UINT or field order corrections. The non-cosmetic changes involve
CustomPath, CustomScan, and CreatePolicyStmt. Feature committers, if the
existing treatments (ignore custom_plans/custom_paths fields; copy/compare
"cmd" string pointer as a scalar) were deliberate, please let me know.
Thanks,
nm
[1] The _equalCreateEventTrigStmt() cosmetic change is relevant as far back as
9.3, but I won't back-patch it further than the others (9.5).
diff --git a/src/backend/nodes/copyfuncs.c b/src/backend/nodes/copyfuncs.c
index 7248440..1c8425d 100644
--- a/src/backend/nodes/copyfuncs.c
+++ b/src/backend/nodes/copyfuncs.c
@@ -647,6 +647,7 @@ _copyCustomScan(const CustomScan *from)
* copy remainder of node
*/
COPY_SCALAR_FIELD(flags);
+ COPY_NODE_FIELD(custom_plans);
COPY_NODE_FIELD(custom_exprs);
COPY_NODE_FIELD(custom_private);
COPY_NODE_FIELD(custom_scan_tlist);
@@ -1925,9 +1926,9 @@ _copyOnConflictExpr(const OnConflictExpr *from)
COPY_SCALAR_FIELD(action);
COPY_NODE_FIELD(arbiterElems);
COPY_NODE_FIELD(arbiterWhere);
+ COPY_SCALAR_FIELD(constraint);
COPY_NODE_FIELD(onConflictSet);
COPY_NODE_FIELD(onConflictWhere);
- COPY_SCALAR_FIELD(constraint);
COPY_SCALAR_FIELD(exclRelIndex);
COPY_NODE_FIELD(exclRelTlist);
@@ -4082,7 +4083,7 @@ _copyCreatePolicyStmt(const CreatePolicyStmt *from)
COPY_STRING_FIELD(policy_name);
COPY_NODE_FIELD(table);
- COPY_SCALAR_FIELD(cmd);
+ COPY_STRING_FIELD(cmd);
COPY_NODE_FIELD(roles);
COPY_NODE_FIELD(qual);
COPY_NODE_FIELD(with_check);
diff --git a/src/backend/nodes/equalfuncs.c b/src/backend/nodes/equalfuncs.c
index 6597dbc..1d6c43c 100644
--- a/src/backend/nodes/equalfuncs.c
+++ b/src/backend/nodes/equalfuncs.c
@@ -759,9 +759,9 @@ _equalOnConflictExpr(const OnConflictExpr *a, const
OnConflictExpr *b)
COMPARE_SCALAR_FIELD(action);
COMPARE_NODE_FIELD(arbiterElems);
COMPARE_NODE_FIELD(arbiterWhere);
+ COMPARE_SCALAR_FIELD(constraint);
COMPARE_NODE_FIELD(onConflictSet);
COMPARE_NODE_FIELD(onConflictWhere);
- COMPARE_SCALAR_FIELD(constraint);
COMPARE_SCALAR_FIELD(exclRelIndex);
COMPARE_NODE_FIELD(exclRelTlist);
@@ -1868,8 +1868,8 @@ _equalCreateEventTrigStmt(const CreateEventTrigStmt *a,
const CreateEventTrigStm
{
COMPARE_STRING_FIELD(trigname);
COMPARE_STRING_FIELD(eventname);
- COMPARE_NODE_FIELD(funcname);
COMPARE_NODE_FIELD(whenclause);
+ COMPARE_NODE_FIELD(funcname);
return true;
}
@@ -2074,7 +2074,7 @@ _equalCreatePolicyStmt(const CreatePolicyStmt *a, const
CreatePolicyStmt *b)
{
COMPARE_STRING_FIELD(policy_name);
COMPARE_NODE_FIELD(table);
- COMPARE_SCALAR_FIELD(cmd);
+ COMPARE_STRING_FIELD(cmd);
COMPARE_NODE_FIELD(roles);
COMPARE_NODE_FIELD(qual);
COMPARE_NODE_FIELD(with_check);
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 86f3214..068724e 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -341,7 +341,7 @@ _outModifyTable(StringInfo str, const ModifyTable *node)
WRITE_NODE_FIELD(arbiterIndexes);
WRITE_NODE_FIELD(onConflictSet);
WRITE_NODE_FIELD(onConflictWhere);
- WRITE_INT_FIELD(exclRelRTI);
+ WRITE_UINT_FIELD(exclRelRTI);
WRITE_NODE_FIELD(exclRelTlist);
}
@@ -591,6 +591,7 @@ _outCustomScan(StringInfo str, const CustomScan *node)
_outScanInfo(str, (const Scan *) node);
WRITE_UINT_FIELD(flags);
+ WRITE_NODE_FIELD(custom_plans);
WRITE_NODE_FIELD(custom_exprs);
WRITE_NODE_FIELD(custom_private);
WRITE_NODE_FIELD(custom_scan_tlist);
@@ -1016,7 +1017,7 @@ _outGroupingFunc(StringInfo str, const GroupingFunc *node)
WRITE_NODE_FIELD(args);
WRITE_NODE_FIELD(refs);
WRITE_NODE_FIELD(cols);
- WRITE_INT_FIELD(agglevelsup);
+ WRITE_UINT_FIELD(agglevelsup);
WRITE_LOCATION_FIELD(location);
}
@@ -1532,9 +1533,9 @@ _outOnConflictExpr(StringInfo str, const OnConflictExpr
*node)
WRITE_ENUM_FIELD(action, OnConflictAction);
WRITE_NODE_FIELD(arbiterElems);
WRITE_NODE_FIELD(arbiterWhere);
+ WRITE_OID_FIELD(constraint);
WRITE_NODE_FIELD(onConflictSet);
WRITE_NODE_FIELD(onConflictWhere);
- WRITE_OID_FIELD(constraint);
WRITE_INT_FIELD(exclRelIndex);
WRITE_NODE_FIELD(exclRelTlist);
}
@@ -1674,6 +1675,7 @@ _outCustomPath(StringInfo str, const CustomPath *node)
_outPathInfo(str, (const Path *) node);
WRITE_UINT_FIELD(flags);
+ WRITE_NODE_FIELD(custom_paths);
WRITE_N
Re: [HACKERS] nodes/*funcs.c inconsistencies
Excerpts from Robert Haas's message of mié abr 18 11:47:37 -0300 2012: > On Mon, Apr 16, 2012 at 6:25 AM, Noah Misch wrote: > > I'd suggest backpatching the ReassignOwnedStmt() bits; the wrong code could > > produce crashes. The rest are for master only. > > Done, in the manner you suggest. Pah. I was just done with it and was about to push it ... -- Álvaro Herrera The PostgreSQL Company - Command Prompt, Inc. PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- 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] nodes/*funcs.c inconsistencies
On Mon, Apr 16, 2012 at 6:25 AM, Noah Misch wrote: > I'd suggest backpatching the ReassignOwnedStmt() bits; the wrong code could > produce crashes. The rest are for master only. Done, in the manner you suggest. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
