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.