Re: Reducing output size of nodeToString

On 04.01.24 00:23, Matthias van de Meent wrote:
> Something like the attached? It splits out into the following
> 0001: basic 'omit default values'

  /* Write an integer field (anything written as ":fldname %d") */
-#define WRITE_INT_FIELD(fldname) \
+#define WRITE_INT_FIELD_DIRECT(fldname) \
         appendStringInfo(str, " :" CppAsString(fldname) " %d", 
node->fldname)
+#define WRITE_INT_FIELD_DEFAULT(fldname, default) \
+       ((node->fldname == default) ? (0) : WRITE_INT_FIELD_DIRECT(fldname))
+#define WRITE_INT_FIELD(fldname) \
+       WRITE_INT_FIELD_DEFAULT(fldname, 0)

Do we need the _DIRECT macros at all?  Could we not combine that into 
the _DEFAULT ones?

I think the way the conditional operator (?:) is written is not 
technically correct C, because one side has an integer result (0) and 
the other a void result (from appendStringInfo()).  Also, this could 
break accidentally even more if the result type of appendStringInfo() 
was changed for some reason.  I think it would be better to write this 
in a more straightforward way like

#define WRITE_INT_FIELD_DEFAULT(fldname, default) \
do { \
     if (node->fldname == default) \
         appendStringInfo(str, " :" CppAsString(fldname) " %d", 
node->fldname); \
while (0)

Relatedly, this

+/* a scaffold function to read an optionally-omitted field */
+#define READ_OPT_SCAFFOLD(fldname, read_field_code, default_value) \
+       if (pg_strtoken_next(":" CppAsString(fldname))) \
+       { \
+               read_field_code; \
+       } \
+       else \
+               local_node->fldname = default_value

would need to be written with a do { } while (0) wrapper around it.


> 0002: reset location and other querystring-related node fields for all
> catalogs of type pg_node_tree.

This goal makes sense, but I think it can be done in a better way.  If 
you look into the area of stringToNode(), stringToNodeWithLocations(), 
and stringToNodeInternal(), there already is support for selectively 
resetting or omitting location fields.  Notably, this works with the 
existing automated knowledge of where the location fields are and 
doesn't require a new hand-maintained table.  I think the way forward 
here would be to apply a similar toggle to nodeToString() (the reverse).


> 0003: add default marking on typmod fields.
> 0004 & 0006: various node fields marked with default() based on
> observed common or initial values of those fields

I think we could get about half the benefit here more automatically, by 
creating a separate type for typmods, like

typedef int32 TypMod;

and then having the node support automatically generate the 
serialization support with a -1 default.

(A similar thing could be applied to the location fields, which would 
allow us to get rid of the current hack of parsing out the name.)

Most of the other defaults I'm doubtful about.  First, we are colliding 
here between the goals of minimizing the storage size and making the 
debug output more readable.  If a Query dump would now omit the 
commandType field if it is CMD_SELECT, I think that would be widely 
confusing, and one would need to check the source code to identify the 
reason.  Also, what if we later decide to change a "default" for a 
field.  Then output between version would differ.  Of course, node 
output does change between versions in general, but these kinds of 
differences would be confusing.  Second, this relies on hand-maintained 
annotations that were created by you presumably through a combination of 
intuition and testing, based on what is in the template database.  Do we 
know whether this matches real-world queries created by users later? 
Also, my experience dealing with the node support over the last little 
while is that these manually maintained exceptions get ossified and 
outdated and create a maintenance headache for the future.


> 0005: truncate trailing 0s from outDatum

Does this significantly affect anything other than the "name" type? 
User views don't usually use the "name" type, so this would have limited 
impact outside of system views.


> 0007 (new): do run-length + gap coding for bitmapset and the various
> integer list types. This saves a surprising amount of bytes.

Can you show examples of this?  How would this affects the ability to 
manually interpret the output?