Re: automating RangeTblEntry node support

On Mon, Mar 11, 2024 at 5:29 AM Peter Eisentraut <peter(at)eisentraut(dot)org>
wrote:

> On 20.02.24 08:57, Peter Eisentraut wrote:
> > On 18.02.24 00:06, Matthias van de Meent wrote:
> >> I'm not sure that the cleanup which is done when changing a RTE's
> >> rtekind is also complete enough for this purpose.
> >> Things like inline_cte_walker change the node->rtekind, which could
> >> leave residual junk data in fields that are currently dropped during
> >> serialization (as the rtekind specifically ignores those fields), but
> >> which would add overhead when the default omission is expected to
> >> handle these fields; as they could then contain junk. It looks like
> >> there is some care about zeroing now unused fields, but I haven't
> >> checked that it covers all cases and fields to the extent required so
> >> that removing this specialized serializer would have zero impact on
> >> size once the default omission patch is committed.
> >>
> >> An additional patch with a single function that for this purpose
> >> clears junk fields from RTEs that changed kind would be appreciated:
> >> it is often hand-coded at those locations the kind changes, but that's
> >> more sensitive to programmer error.
> >
> > Yes, interesting idea. Or maybe an assert-like function that checks an
> > existing structure for consistency. Or maybe both. I'll try this out.
> >
> > In the meantime, if there are no remaining concerns, I propose to commit
> > the first two patches
> >
> > Remove custom Constraint node read/write implementations
> > Remove custom _jumbleRangeTblEntry()
>
> After a few side quests, here is an updated patch set. (I had committed
> the first of the two patches mentioned above, but not yet the second one.)
>
> v3-0001-Remove-obsolete-comment.patch
> v3-0002-Improve-comment.patch
>
> These just update a few comments around the RangeTblEntry definition.
>
> v3-0003-Reformat-some-node-comments.patch
> v3-0004-Remove-custom-_jumbleRangeTblEntry.patch
>
> This is pretty much the same patch as before. I have now split it up to
> first reformat the comments to make room for the node annotations. This
> patch is now also pgindent-proof. After some side quest discussions,
> the set of fields to jumble seems correct now, so commit message
> comments to the contrary have been dropped.
>
> v3-0005-Make-RangeTblEntry-dump-order-consistent.patch
>
> I separated that from the 0008 patch below. I think it useful even if
> we don't go ahead with 0008 now, for example in dumps from the debugger,
> and just in general to keep everything more consistent.
>
> v3-0006-WIP-AssertRangeTblEntryIsValid.patch
>
> This is in response to some of the discussions where there was some
> doubt whether all fields are always filled and cleared correctly when
> the RTE kind is changed. Seems correct as far as this goes. I didn't
> know of a good way to hook this in, so I put it into the write/read
> functions, which is obviously a bit weird if I'm proposing to remove
> them later. Consider it a proof of concept.
>
> v3-0007-Simplify-range_table_mutator_impl-and-range_table.patch
> v3-0008-WIP-Remove-custom-RangeTblEntry-node-read-write-i.patch
>
> At this point, I'm not too stressed about pressing forward with these
> last two patches. We can look at them again perhaps if we make progress
> on a more compact node output format. When I started this thread, I had
> a lot of questions about various details about the RangeTblEntry struct,
> and we have achieved many answers during the discussions, so I'm happy
> with the progress. So for PG17, I'd like to just do patches 0001..0005.
>

Patches 1 thru 5 look good to me

cheers

andrew