Обсуждение: 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 *parent_ec pg_node_attr(copy_as_scalar, equal_ignore, read_write_ignore);

     /*
      * cache space for cost and selectivity
@@ -2500,13 +2503,13 @@ typedef struct RestrictInfo
      */

     /* EquivalenceClass containing lefthand */
-    EquivalenceClass *left_ec pg_node_attr(equal_ignore, read_write_ignore);
+    EquivalenceClass *left_ec pg_node_attr(copy_as_scalar, equal_ignore, read_write_ignore);
     /* EquivalenceClass containing righthand */
-    EquivalenceClass *right_ec pg_node_attr(equal_ignore, read_write_ignore);
+    EquivalenceClass *right_ec pg_node_attr(copy_as_scalar, equal_ignore, read_write_ignore);
     /* EquivalenceMember for lefthand */
-    EquivalenceMember *left_em pg_node_attr(equal_ignore);
+    EquivalenceMember *left_em pg_node_attr(copy_as_scalar, equal_ignore);
     /* EquivalenceMember for righthand */
-    EquivalenceMember *right_em pg_node_attr(equal_ignore);
+    EquivalenceMember *right_em pg_node_attr(copy_as_scalar, equal_ignore);

     /*
      * List of MergeScanSelCache structs.  Those aren't Nodes, so hard to

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.

Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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

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;
         }

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.

Peter Eisentraut <peter.eisentraut@enterprisedb.com> 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