Re: [HACKERS] Inadequate infrastructure for NextValueExpr
> "Thomas" == Thomas Munro writes: >> [...] >> T_NamedTuplestoreScan can be produced by outfuncs.c with tagname >> NAMEDTUPLESTORESCAN but that tagname is not recognized by readfuncs.c >> [...] >> >> That revealed a defect in commit >> 18ce3a4ab22d2984f8540ab480979c851dae5338 which I think should be >> corrected with something like the attached, though I'm not sure if >> it's possible to reach it. Thomas> Adding Kevin and Andrew G. Thoughts on whether this is a Thomas> defect that should be corrected with something like Thomas> read-namedtuplestorescan.patch? It's a defect that should probably be corrected for consistency, though at present it looks like it's not actually possible to reach the code. The patch looks good to me though I've not tested it. Kevin, you want to take it? Or shall I deal with it? -- Andrew (irc:RhodiumToad) -- 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] Inadequate infrastructure for NextValueExpr
On Fri, Jul 14, 2017 at 1:46 PM, Thomas Munro wrote: > [...] > T_NamedTuplestoreScan can be produced by outfuncs.c with tagname > NAMEDTUPLESTORESCAN but that tagname is not recognized by readfuncs.c > [...] > > That revealed a defect in commit > 18ce3a4ab22d2984f8540ab480979c851dae5338 which I think should be > corrected with something like the attached, though I'm not sure if > it's possible to reach it. Adding Kevin and Andrew G. Thoughts on whether this is a defect that should be corrected with something like read-namedtuplestorescan.patch? -- Thomas Munro http://www.enterprisedb.com -- 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] Inadequate infrastructure for NextValueExpr
On Wed, Jul 26, 2017 at 4:04 PM, Thomas Munro wrote: > On Wed, Jul 26, 2017 at 6:35 AM, Robert Haas wrote: >> That's pretty cool. Per long-standing precedent, anything we use in a >> build needs to be in Perl, not Python, but I think it would be great >> to fix all of these (or the script) and then add this to our standard >> build process. It would be *great* to stop making mistakes like this. > > Ok, here is a Perl version. It is literally my first Perl program so > I probably got all those squiggles wrong. Ouput as of today: Argh, let me try that again with a copy-and-paste error corrected: $ ./src/tools/check_node_support_code.pl T_ObjectWithArgs (category PARSE TREE NODES) not handled in outfuncs.c T_AccessPriv (category PARSE TREE NODES) not handled in outfuncs.c T_CreateOpClassItem (category PARSE TREE NODES) not handled in outfuncs.c T_FunctionParameter (category PARSE TREE NODES) not handled in outfuncs.c T_InferClause (category PARSE TREE NODES) not handled in outfuncs.c T_OnConflictClause (category PARSE TREE NODES) not handled in outfuncs.c T_RoleSpec (category PARSE TREE NODES) not handled in outfuncs.c T_PartitionCmd (category PARSE TREE NODES) not handled in outfuncs.c T_NamedTuplestoreScan is written by outfuncs.c as NAMEDTUPLESTORESCAN but that name is not recognized by readfuncs.c T_Alias (category PRIMITIVE NODES) not handled in ruleutils.c:get_rule_expr T_RangeVar (category PRIMITIVE NODES) not handled in ruleutils.c:get_rule_expr T_Expr (category PRIMITIVE NODES) not handled in ruleutils.c:get_rule_expr T_CaseWhen (category PRIMITIVE NODES) not handled in ruleutils.c:get_rule_expr T_TargetEntry (category PRIMITIVE NODES) not handled in ruleutils.c:get_rule_expr T_RangeTblRef (category PRIMITIVE NODES) not handled in ruleutils.c:get_rule_expr T_JoinExpr (category PRIMITIVE NODES) not handled in ruleutils.c:get_rule_expr T_FromExpr (category PRIMITIVE NODES) not handled in ruleutils.c:get_rule_expr T_OnConflictExpr (category PRIMITIVE NODES) not handled in ruleutils.c:get_rule_expr T_IntoClause (category PRIMITIVE NODES) not handled in ruleutils.c:get_rule_expr -- Thomas Munro http://www.enterprisedb.com check-node-support-code-v2.patch Description: Binary data -- 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] Inadequate infrastructure for NextValueExpr
On Wed, Jul 26, 2017 at 6:35 AM, Robert Haas wrote: > That's pretty cool. Per long-standing precedent, anything we use in a > build needs to be in Perl, not Python, but I think it would be great > to fix all of these (or the script) and then add this to our standard > build process. It would be *great* to stop making mistakes like this. Ok, here is a Perl version. It is literally my first Perl program so I probably got all those squiggles wrong. Ouput as of today: $ ./src/tools/check_node_support_code.pl T_ObjectWithArgs (category PARSE TREE NODES) not handled in outfuncs.c T_AccessPriv (category PARSE TREE NODES) not handled in outfuncs.c T_CreateOpClassItem (category PARSE TREE NODES) not handled in outfuncs.c T_FunctionParameter (category PARSE TREE NODES) not handled in outfuncs.c T_InferClause (category PARSE TREE NODES) not handled in outfuncs.c T_OnConflictClause (category PARSE TREE NODES) not handled in outfuncs.c T_RoleSpec (category PARSE TREE NODES) not handled in outfuncs.c T_PartitionCmd (category PARSE TREE NODES) not handled in outfuncs.c T_NamedTuplestoreScan is written by outfuncs.c as NAMEDTUPLESTORESCAN but that name is not recognized by readfuncs.c T_Alias (category PRIMITIVE NODES) not handled in equalfuncs.c T_RangeVar (category PRIMITIVE NODES) not handled in equalfuncs.c T_Expr (category PRIMITIVE NODES) not handled in equalfuncs.c T_CaseWhen (category PRIMITIVE NODES) not handled in equalfuncs.c T_TargetEntry (category PRIMITIVE NODES) not handled in equalfuncs.c T_RangeTblRef (category PRIMITIVE NODES) not handled in equalfuncs.c T_JoinExpr (category PRIMITIVE NODES) not handled in equalfuncs.c T_FromExpr (category PRIMITIVE NODES) not handled in equalfuncs.c T_OnConflictExpr (category PRIMITIVE NODES) not handled in equalfuncs.c T_IntoClause (category PRIMITIVE NODES) not handled in equalfuncs.c -- Thomas Munro http://www.enterprisedb.com check-node-support-code-v1.patch Description: Binary data -- 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] Inadequate infrastructure for NextValueExpr
On Thu, Jul 13, 2017 at 9:46 PM, Thomas Munro wrote: > I wrote a script to cross-check various node handling functions and it told > me: > > T_NextValueExpr not handled in outfuncs.c > T_ObjectWithArgs not handled in outfuncs.c > T_AccessPriv not handled in outfuncs.c > T_CreateOpClassItem not handled in outfuncs.c > T_FunctionParameter not handled in outfuncs.c > T_InferClause not handled in outfuncs.c > T_OnConflictClause not handled in outfuncs.c > T_RoleSpec not handled in outfuncs.c > T_PartitionCmd not handled in outfuncs.c > T_NamedTuplestoreScan can be produced by outfuncs.c with tagname > NAMEDTUPLESTORESCAN but that tagname is not recognized by readfuncs.c > T_Alias not handled in ruleutils.c > T_RangeVar not handled in ruleutils.c > T_Expr not handled in ruleutils.c > T_CaseWhen not handled in ruleutils.c > T_TargetEntry not handled in ruleutils.c > T_RangeTblRef not handled in ruleutils.c > T_JoinExpr not handled in ruleutils.c > T_FromExpr not handled in ruleutils.c > T_OnConflictExpr not handled in ruleutils.c > T_IntoClause not handled in ruleutils.c > T_NextValueExpr not handled in ruleutils.c That's pretty cool. Per long-standing precedent, anything we use in a build needs to be in Perl, not Python, but I think it would be great to fix all of these (or the script) and then add this to our standard build process. It would be *great* to stop making mistakes like this. -- 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
Re: [HACKERS] Inadequate infrastructure for NextValueExpr
On Fri, Jul 14, 2017 at 9:34 AM, Tom Lane wrote: > Somebody decided they could add a new primnode type without bothering to > build out very much infrastructure for it. Thus: > > regression=# create table foo (f1 int, f2 int generated always as identity); > CREATE TABLE > regression=# insert into foo values(1); > INSERT 0 1 > regression=# explain verbose insert into foo values(1); > ERROR: unrecognized node type: 146 > > because ruleutils.c has never heard of NextValueExpr. The lack of > outfuncs/readfuncs support for it is rather distressing as well. > That doesn't break parallel queries today, because (I think) > you can't get one of these nodes in a parallelizable query, but it > is going to cause problems for debugging. It will also break > (more or less) pg_stat_statements. I also wonder whether costsize.c > oughtn't be charging some estimated execution cost for it. I wrote a script to cross-check various node handling functions and it told me: T_NextValueExpr not handled in outfuncs.c T_ObjectWithArgs not handled in outfuncs.c T_AccessPriv not handled in outfuncs.c T_CreateOpClassItem not handled in outfuncs.c T_FunctionParameter not handled in outfuncs.c T_InferClause not handled in outfuncs.c T_OnConflictClause not handled in outfuncs.c T_RoleSpec not handled in outfuncs.c T_PartitionCmd not handled in outfuncs.c T_NamedTuplestoreScan can be produced by outfuncs.c with tagname NAMEDTUPLESTORESCAN but that tagname is not recognized by readfuncs.c T_Alias not handled in ruleutils.c T_RangeVar not handled in ruleutils.c T_Expr not handled in ruleutils.c T_CaseWhen not handled in ruleutils.c T_TargetEntry not handled in ruleutils.c T_RangeTblRef not handled in ruleutils.c T_JoinExpr not handled in ruleutils.c T_FromExpr not handled in ruleutils.c T_OnConflictExpr not handled in ruleutils.c T_IntoClause not handled in ruleutils.c T_NextValueExpr not handled in ruleutils.c It classifies all node types into categories PLAN NODES, PRIMITIVE NODES, ... using the per-group comments in nodes.h, and then checks some rules that I inferred (obviously incorrectly) about which of those categories should be handled by copy, out, equal, read and get_rule_expr functions and also checks that readfuncs.c and outfuncs.c agree on the name string. It needs some work though: anyone got any ideas on how to improve the categories and rules to make it right? To use this approach, it would need to be the case that each checked function exhaustively handles a subset of node tags that can be described by listing those categories. That revealed a defect in commit 18ce3a4ab22d2984f8540ab480979c851dae5338 which I think should be corrected with something like the attached, though I'm not sure if it's possible to reach it. It would be nice to do something more mechanised for this type of code... -- Thomas Munro http://www.enterprisedb.com read-namedtuplestorescan.patch Description: Binary data #!/usr/bin/env python # # Cross-check node handling code in PostgreSQL looking for bugs. import re def scan_node_categories(path): """Find all type tags and put them into categories.""" results = {} with open(path) as file: current_list = [] results["INVALID"] = current_list for line in file: matches = re.search(r"TAGS FOR ([A-Z].*(NODES|STUFF))", line) if matches: category = matches.group(1) current_list = [] results[category] = current_list continue matches = re.search(r"(T_[a-zA-Z0-9_]+)", line) if matches: assert current_list != None tag = matches.group(1) current_list.append(tag) return results def scan_switch(path, function): """Find all cases handled by "function" in translation unit "path".""" cases = [] in_function = False with open(path) as file: for line in file: if line.startswith(function + "("): in_function = True elif line.startswith("}"): in_function = False elif in_function: matches = re.search(r"case (T_.+):", line) if matches: cases.append(matches.group(1)) return cases def scan_read_tagnames(path): """Find all tagnames that readfuncs.c recognizes.""" results = [] with open(path) as file: for line in file: matches = re.search("MATCH\(\"([A-Z]+)\", ([0-9]+)", line) if matches: tagname = matches.group(1) length = int(matches.group(2)) results.append(tagname) if length != len(tagname): print "Unexpected length:", line return results def scan_out_tagnames(path): """Learn what tagname outfuncs.c uses to output each type.""" results = {} current_tag = None with open(path) as file: for line in file: matches = re.match(r"_out.*, const ([^ ]+) \*", line) if matches: # see makeNode macro definition: typedef Foo must have enum T_Foo current_tag = "T_" + matches.group(1) elif line.startswith("}"): current_tag = None elif
[HACKERS] Inadequate infrastructure for NextValueExpr
Somebody decided they could add a new primnode type without bothering to build out very much infrastructure for it. Thus: regression=# create table foo (f1 int, f2 int generated always as identity); CREATE TABLE regression=# insert into foo values(1); INSERT 0 1 regression=# explain verbose insert into foo values(1); ERROR: unrecognized node type: 146 because ruleutils.c has never heard of NextValueExpr. The lack of outfuncs/readfuncs support for it is rather distressing as well. That doesn't break parallel queries today, because (I think) you can't get one of these nodes in a parallelizable query, but it is going to cause problems for debugging. It will also break (more or less) pg_stat_statements. I also wonder whether costsize.c oughtn't be charging some estimated execution cost for it. 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