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