From: | Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> |
---|---|
To: | Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> |
Cc: | David Rowley <dgrowleyml(at)gmail(dot)com>, pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: automatically generating node support functions |
Date: | 2022-07-06 10:28:31 |
Message-ID: | b53b92e0-1153-a31a-8c52-157fa2e07771@enterprisedb.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
The new patch addresses almost all of these issues.
> Also, I share David's upthread allergy to the option names
> "path_hackN" and to documenting those only inside the conversion
> script.
I have given these real names now and documented them with the other
attributes.
> BTW, I think this: "Unknown attributes are ignored" is a seriously
> bad idea; it will allow typos to escape detection.
fixed
(I have also changed the inside of pg_node_attr to be comma-separated,
rather than space-separated. This matches better how attribute-type
things look in C.)
> I think we ought to put it into the *nodes.h headers as much as
> possible, perhaps like this:
>
> typedef struct A_Const pg_node_attr(custom_copy)
> { ...
done
> So I propose that we handle these things via struct-level pg_node_attr
> markers, rather than node-type lists embedded in the script:
>
> abstract_types
> no_copy
> no_read_write
> no_read
> custom_copy
> custom_readwrite
done (no_copy is actually no_copy_equal, hence renamed)
> The hacks for scalar-copying EquivalenceClass*, EquivalenceMember*,
> struct CustomPathMethods*, and CustomScan.methods should be replaced
> with "pg_node_attr(copy_as_scalar)" labels on affected fields.
Hmm, at least for Equivalence..., this is repeated a bunch of times for
each field. I don't know if this is really a property of the type or
something you can choose for each field? [not changed in v7 patch]
> I wonder whether this:
>
> # We do not support copying Path trees, mainly
> # because the circular linkages between RelOptInfo
> # and Path nodes can't be handled easily in a
> # simple depth-first traversal.
>
> couldn't be done better by inventing an inheritable no_copy attr
> to attach to the Path supertype. Or maybe it'd be okay to just
> automatically inherit the no_xxx properties from the supertype?
This is an existing comment in copyfuncs.c. I haven't looked into it
any further.
> I don't terribly like the ad-hoc mechanism for not comparing
> CoercionForm fields. OTOH, I am not sure whether replacing it
> with per-field equal_ignore attrs would be better; there's at least
> an argument that that invites bugs of omission. But implementing
> this with an uncommented test deep inside a script that most hackers
> should not need to read is not good. On the whole I'd lean towards
> the equal_ignore route.
The definition of CoercionForm in primnodes.h says that the comparison
behavior is a property of the type, so it needs to be handled somewhere
centrally, not on each field. [not changed in v7 patch]
> I'm confused by the "various field types to ignore" at the end
> of the outfuncs/readfuncs code. Do we really ignore those now?
> How could that be safe? If it is safe, wouldn't it be better
> to handle that with per-field pg_node_attrs? Silently doing
> what might be the wrong thing doesn't seem good.
I have replaced these with explicit ignore markings in pathnodes.h
(PlannerGlobal, PlannerInfo, RelOptInfo). (This could then use a bit
more rearranging some of the per-field comments.)
> * copyfuncs.switch.c and equalfuncs.switch.c are missing trailing
> newlines.
fixed
> * pgindent is not very happy with a lot of your comments in *nodes.h.
fixed
> * I think we should add explicit dependencies in backend/nodes/Makefile,
> along the lines of
>
> copyfuncs.o: copyfuncs.c copyfuncs.funcs.c copyfuncs.switch.c
>
> Otherwise the whole thing is a big gotcha for anyone not using
> --enable-depend.
fixed -- I think, could use more testing
Attachment | Content-Type | Size |
---|---|---|
v7-0001-Automatically-generate-node-support-functions.patch | text/plain | 92.7 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Peter Eisentraut | 2022-07-06 10:30:49 | Re: automatically generating node support functions |
Previous Message | Dagfinn Ilmari Mannsåker | 2022-07-06 09:57:53 | Re: Logging query parmeters in auto_explain |