Horton

Thread

Commits

  1. Remove gen_node_support.pl's special treatment of EquivalenceClasses.

  2. Add some error cross-checks to gen_node_support.pl.

  1. Removing another gen_node_support.pl special case

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-11-27T01:39:05Z

    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

  2. Re: Removing another gen_node_support.pl special case

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-11-28T16:25:13Z

    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.

  3. Re: Removing another gen_node_support.pl special case

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-11-28T16:39:22Z

    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

  4. Re: Removing another gen_node_support.pl special case

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-11-29T21:34:30Z

    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

  5. Re: Removing another gen_node_support.pl special case

    Peter Eisentraut <peter.eisentraut@enterprisedb.com> — 2022-12-02T13:00:55Z

    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.

  6. Re: Removing another gen_node_support.pl special case

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-12-02T20:25:00Z

    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