Removing another gen_node_support.pl special case
I got confused about how we were managing EquivalenceClass pointers in the copy/equal infrastructure, and it took me awhile to remember that the reason it works is that gen_node_support.pl has hard-wired knowledge about that. I think that's something we'd be best off dropping in favor of explicit annotations on affected fields. Hence, I propose the attached. This results in zero change in the generated copy/equal code. regards, tom lane diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl index b6f086e262..9f7b4b833f 100644 --- a/src/backend/nodes/gen_node_support.pl +++ b/src/backend/nodes/gen_node_support.pl @@ -166,9 +166,6 @@ my @custom_read_write; # Track node types with manually assigned NodeTag numbers. my %manual_nodetag_number; -# EquivalenceClasses are never moved, so just shallow-copy the pointer -push @scalar_types, qw(EquivalenceClass* EquivalenceMember*); - # This is a struct, so we can copy it by assignment. Equal support is # currently not required. push @scalar_types, qw(QualCost); @@ -458,9 +455,14 @@ foreach my $infile (@ARGV) && $attr !~ /^copy_as\(\w+\)$/ && $attr !~ /^read_as\(\w+\)$/ && !elem $attr, -qw(equal_ignore equal_ignore_if_zero read_write_ignore -write_only_relids write_only_nondefault_pathtarget write_only_req_outer) - ) +qw(copy_as_scalar +equal_as_scalar +equal_ignore +equal_ignore_if_zero +read_write_ignore +write_only_relids +write_only_nondefault_pathtarget +write_only_req_outer)) { die "$infile:$lineno: unrecognized attribute \"$attr\"\n"; @@ -692,6 +694,8 @@ _equal${n}(const $n *a, const $n *b) # extract per-field attributes my $array_size_field; my $copy_as_field; + my $copy_as_scalar = 0; + my $equal_as_scalar = 0; foreach my $a (@a) { if ($a =~ /^array_size\(([\w.]+)\)$/) @@ -702,19 +706,41 @@ _equal${n}(const $n *a, const $n *b) { $copy_as_field = $1; } + elsif ($a eq 'copy_as_scalar') + { +$copy_as_scalar = 1; + } + elsif ($a eq 'equal_as_scalar') + { +$equal_as_scalar = 1; + } elsif ($a eq 'equal_ignore') { $equal_ignore = 1; } } - # override type-specific copy method if copy_as is specified + # override type-specific copy method if requested if (defined $copy_as_field) { print $cff "\tnewnode->$f = $copy_as_field;\n" unless $copy_ignore; $copy_ignore = 1; } + elsif ($copy_as_scalar) + { + print $cff "\tCOPY_SCALAR_FIELD($f);\n" + unless $copy_ignore; + $copy_ignore = 1; + } + + # override type-specific equal method if requested + if ($equal_as_scalar) + { + print $eff "\tCOMPARE_SCALAR_FIELD($f);\n" + unless $equal_ignore; + $equal_ignore = 1; + } # select instructions by field type if ($t eq 'char*') diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h index a80f43e540..d98f2c91d9 100644 --- a/src/include/nodes/nodes.h +++ b/src/include/nodes/nodes.h @@ -86,6 +86,12 @@ typedef enum NodeTag * * - copy_as(VALUE): In copyObject(), replace the field's value with VALUE. * + * - copy_as_scalar: In copyObject(), copy the field's pointer value + * even if it is a node-type pointer. + * + * - equal_as_scalar: In equal(), compare the field by pointer equality + * even if it is a node-type pointer. + * * - equal_ignore: Ignore the field for equality. * * - equal_ignore_if_zero: Ignore the field for equality if it is zero. diff --git a/src/include/nodes/pathnodes.h b/src/include/nodes/pathnodes.h index a544b313d3..885ad42327 100644 --- a/src/include/nodes/pathnodes.h +++ b/src/include/nodes/pathnodes.h @@ -1273,7 +1273,9 @@ typedef struct StatisticExtInfo * * NB: EquivalenceClasses are never copied after creation. Therefore, * copyObject() copies pointers to them as pointers, and equal() compares - * pointers to EquivalenceClasses via pointer equality. + * pointers to EquivalenceClasses via pointer equality. This is implemented + * by putting copy_as_scalar and equal_as_scalar attributes on fields that + * are pointers to EquivalenceClasses. The same goes for EquivalenceMembers. */ typedef struct EquivalenceClass { @@ -1364,7 +1366,8 @@ typedef struct PathKey NodeTag type; - EquivalenceClass *pk_eclass; /* the value that is ordered */ + /* the value that is ordered */ + EquivalenceClass *pk_eclass pg_node_attr(copy_as_scalar, equal_as_scalar); Oid pk_opfamily; /* btree opfamily defining the ordering */ int pk_strategy; /* sort direction (ASC or DESC) */ bool pk_nulls_first; /* do NULLs come before normal values? */ @@ -2472,7 +2475,7 @@ typedef struct RestrictInfo * Generating EquivalenceClass. This field is NULL unless clause is * potentially redundant. */ - EquivalenceClass *parent_ec pg_node_attr(equal_ignore, read_write_ignore); + EquivalenceClass *paren
Re: Removing another gen_node_support.pl special case
On 27.11.22 02:39, Tom Lane wrote: I got confused about how we were managing EquivalenceClass pointers in the copy/equal infrastructure, and it took me awhile to remember that the reason it works is that gen_node_support.pl has hard-wired knowledge about that. I think that's something we'd be best off dropping in favor of explicit annotations on affected fields. Hence, I propose the attached. This results in zero change in the generated copy/equal code. I suppose the question is whether this behavior is something that is a property of the EquivalenceClass type as such or something that is specific to each individual field.
Re: Removing another gen_node_support.pl special case
Peter Eisentraut writes: > On 27.11.22 02:39, Tom Lane wrote: >> I got confused about how we were managing EquivalenceClass pointers >> in the copy/equal infrastructure, and it took me awhile to remember >> that the reason it works is that gen_node_support.pl has hard-wired >> knowledge about that. I think that's something we'd be best off >> dropping in favor of explicit annotations on affected fields. >> Hence, I propose the attached. This results in zero change in >> the generated copy/equal code. > I suppose the question is whether this behavior is something that is a > property of the EquivalenceClass type as such or something that is > specific to each individual field. That's an interesting point, but what I'm on about is that I don't want the behavior buried in gen_node_support.pl. I think there's a reasonable argument to be made that equal_as_scalar *is* a field-level property not a node-level property. I agree that for the copy case you could argue it differently, and I also agree that it seems error-prone to have to remember to label fields this way. I notice that EquivalenceClass is already marked as no_copy_equal, which means that gen_node_support.pl can know that emitting a recursive node-copy or node-compare request is a bad idea. What do you think of using the patch as it stands, plus a cross-check that we don't emit COPY_NODE_FIELD or COMPARE_NODE_FIELD if the target node type is no_copy or no_equal? This is different from just silently applying scalar copy/equal, in that (a) it's visibly under the programmer's control, and (b) it's not hard to imagine wanting to use other solutions such as copy_as(NULL). (More generally, I suspect that there are other useful cross-checks gen_node_support.pl could be making. I had a to-do item to think about that, but it didn't get to the top of the list yet.) regards, tom lane
Re: Removing another gen_node_support.pl special case
I wrote: > I notice that EquivalenceClass is already marked as no_copy_equal, > which means that gen_node_support.pl can know that emitting a > recursive node-copy or node-compare request is a bad idea. What > do you think of using the patch as it stands, plus a cross-check > that we don't emit COPY_NODE_FIELD or COMPARE_NODE_FIELD if the > target node type is no_copy or no_equal? Concretely, it seems like something like the attached could be useful, independently of the other change. regards, tom lane diff --git a/src/backend/nodes/gen_node_support.pl b/src/backend/nodes/gen_node_support.pl index b6f086e262..fc5b6721d6 100644 --- a/src/backend/nodes/gen_node_support.pl +++ b/src/backend/nodes/gen_node_support.pl @@ -123,6 +123,8 @@ my @no_equal; my @no_read; # node types we don't want read/write support for my @no_read_write; +# node types that have handmade read/write support +my @special_read_write; # node types we don't want any support functions for, just node tags my @nodetag_only; @@ -152,9 +154,12 @@ my @extra_tags = qw( # since we won't use its internal structure here anyway. push @node_types, qw(List); # Lists are specially treated in all four support files, too. -push @no_copy, qw(List); -push @no_equal, qw(List); -push @no_read_write, qw(List); +# (Ideally we'd mark List as "special copy/equal" not "no copy/equal". +# But until there's other use-cases for that, just hot-wire the tests +# that would need to distinguish.) +push @no_copy,qw(List); +push @no_equal, qw(List); +push @special_read_write, qw(List); # Nodes with custom copy/equal implementations are skipped from # .funcs.c but need case statements in .switch.c. @@ -338,16 +343,7 @@ foreach my $infile (@ARGV) } elsif ($attr eq 'special_read_write') { - # This attribute is called - # "special_read_write" because there is - # special treatment in outNode() and - # nodeRead() for these nodes. For this - # script, it's the same as - # "no_read_write", but calling the - # attribute that externally would probably - # be confusing, since read/write support - # does in fact exist. - push @no_read_write, $in_struct; + push @special_read_write, $in_struct; } elsif ($attr =~ /^nodetag_number\((\d+)\)$/) { @@ -786,6 +782,17 @@ _equal${n}(const $n *a, const $n *b) elsif (($t =~ /^(\w+)\*$/ or $t =~ /^struct\s+(\w+)\*$/) and elem $1, @node_types) { + die + "node type \"$1\" lacks copy support, which is required for struct \"$n\" field \"$f\"\n" + if (elem $1, @no_copy or elem $1, @nodetag_only) + and $1 ne 'List' + and !$copy_ignore; + die + "node type \"$1\" lacks equal support, which is required for struct \"$n\" field \"$f\"\n" + if (elem $1, @no_equal or elem $1, @nodetag_only) + and $1 ne 'List' + and !$equal_ignore; + print $cff "\tCOPY_NODE_FIELD($f);\n"unless $copy_ignore; print $eff "\tCOMPARE_NODE_FIELD($f);\n" unless $equal_ignore; } @@ -851,6 +858,7 @@ foreach my $n (@node_types) next if elem $n, @abstract_types; next if elem $n, @nodetag_only; next if elem $n, @no_read_write; + next if elem $n, @special_read_write; my $no_read = (elem $n, @no_read); @@ -1083,6 +1091,14 @@ _read${n}(void) elsif (($t =~ /^(\w+)\*$/ or $t =~ /^struct\s+(\w+)\*$/) and elem $1, @node_types) { + die + "node type \"$1\" lacks write support, which is required for struct \"$n\" field \"$f\"\n" + if (elem $1, @no_read_write or elem $1, @nodetag_only); + die + "node type \"$1\" lacks read support, which is required for struct \"$n\" field \"$f\"\n" + if (elem $1, @no_read or elem $1, @nodetag_only) + and !$no_read; + print $off "\tWRITE_NODE_FIELD($f);\n"; print $rff "\tREAD_NODE_FIELD($f);\n" unless $no_read; }
Re: Removing another gen_node_support.pl special case
On 29.11.22 22:34, Tom Lane wrote: I wrote: I notice that EquivalenceClass is already marked as no_copy_equal, which means that gen_node_support.pl can know that emitting a recursive node-copy or node-compare request is a bad idea. What do you think of using the patch as it stands, plus a cross-check that we don't emit COPY_NODE_FIELD or COMPARE_NODE_FIELD if the target node type is no_copy or no_equal? Concretely, it seems like something like the attached could be useful, independently of the other change. Yes, right now you can easily declare things that don't make sense. Cross-checks like these look useful.
Re: Removing another gen_node_support.pl special case
Peter Eisentraut writes: > On 29.11.22 22:34, Tom Lane wrote: >> Concretely, it seems like something like the attached could be >> useful, independently of the other change. > Yes, right now you can easily declare things that don't make sense. > Cross-checks like these look useful. Checking my notes from awhile back, there was one other cross-check that I thought was pretty high-priority: verifying that array_size fields precede their array fields. Without that, a read function will fail entirely, and a compare function might index off the end of an array depending on which array-size field it chooses to believe. It seems like an easy mistake to make, too. I added that and pushed. regards, tom lane