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.

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

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