On Mon, Mar 11, 2024 at 5:29 AM Peter Eisentraut <pe...@eisentraut.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
