On Thu, 15 Feb 2024 at 13:59, Peter Eisentraut <pe...@eisentraut.org> wrote: > > Thanks, this patch set is a good way to incrementally work through these > changes. > > I have looked at > v4-0001-pg_node_tree-Omit-serialization-of-fields-with-de.patch today. > Here are my thoughts: > > I believe we had discussed offline to not omit enum fields with value 0 > (WRITE_ENUM_FIELD). This is because the values of enum fields are > implementation artifacts, and this could be confusing for readers.
Thanks for reminding me, I didn't remember this when I worked on updating the patchset. I'll update this soon. > I have some concerns about the round-trippability of float values. If > we do, effectively, if (node->fldname != 0.0), then I think this would > also match negative zero, but when we read it back it, it would get > assigned positive zero. Maybe there are other edge cases like this. > Might be safer to not mess with this. That's a good point. Would an additional check that the sign of the field equals the default's sign be enough for this? As for other cases, I'm not sure we currently want to support non-normal floats, even if it is technically possible to do the round-trip in the current format. > On the reading side, the macro nesting has gotten a bit out of hand. :) > We had talked earlier in the thread about the _DIRECT macros and you > said there were left over from something else you want to try, but I see > nothing else in this patch set uses this. I think this could all be > much simpler, like (omitting required punctuation) [...] > Not only is this simpler, but it might also have better performance, > because we don't have separate pg_strtok_next() and pg_strtok() calls in > sequence. Good points. I'll see what I can do here. Kind regards, Matthias van de Meent Neon (https://neon.tech)
