On 22.02.24 16:07, Matthias van de Meent wrote:
On Thu, 22 Feb 2024 at 13:37, Matthias van de Meent
<boekewurm+postg...@gmail.com> wrote:

On Mon, 19 Feb 2024 at 14:19, Matthias van de Meent
<boekewurm+postg...@gmail.com> wrote:
Attached the updated version of the patch on top of 5497daf3, which
incorporates this last round of feedback.

Now attached rebased on top of 93db6cbd to fix conflicts with fbc93b8b
and an issue in the previous patchset: I attached one too many v3-0001
from a previous patch I worked on.

... and now with a fix for not overwriting newly deserialized location
attributes with -1, which breaks test output for
READ_WRITE_PARSE_PLAN_TREES installations. Again, no other significant
changes since the patch of last Monday.

* v7-0002-pg_node_tree-Don-t-store-query-text-locations-in-.patch

This patch looks much more complicated than I was expecting. I had suggested to model this after stringToNodeWithLocations(). This uses a global variable to toggle the mode. Your patch creates a function nodeToStringNoQLocs() -- why the different naming scheme? -- and passes the flag down as an argument to all the output functions. I mean, in a green field, avoiding global variables can be sensible, of course, but I think in this limited scope here it would really be better to keep the code for the two directions read and write the same.

Attached is a small patch that shows what I had in mind. (It doesn't contain any callers, but your patch shows where all those would go.)


* v7-0003-gen_node_support.pl-Mark-location-fields-as-type-.patch

This looks sensible, but maybe making Location a global type is a bit much? Maybe something more specific like ParseLocation, or ParseLoc, to keep it under 12 characters.
From dee403f637f279813f0711bbc64c365cba82c18c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <pe...@eisentraut.org>
Date: Mon, 11 Mar 2024 14:07:52 +0100
Subject: [PATCH] Add nodeToStringWithoutLocations()

---
 src/backend/nodes/outfuncs.c | 30 +++++++++++++++++++++++++++++-
 src/include/nodes/nodes.h    |  1 +
 2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 29cbc83bd9..ea0a6cb94d 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -25,6 +25,8 @@
 #include "nodes/pg_list.h"
 #include "utils/datum.h"
 
+static bool write_location_fields = true;
+
 static void outChar(StringInfo str, char c);
 static void outDouble(StringInfo str, double d);
 
@@ -88,7 +90,7 @@ static void outDouble(StringInfo str, double d);
 
 /* Write a parse location field (actually same as INT case) */
 #define WRITE_LOCATION_FIELD(fldname) \
-       appendStringInfo(str, " :" CppAsString(fldname) " %d", node->fldname)
+       appendStringInfo(str, " :" CppAsString(fldname) " %d", 
write_location_fields ? node->fldname : -1)
 
 /* Write a Node field */
 #define WRITE_NODE_FIELD(fldname) \
@@ -770,6 +772,32 @@ nodeToString(const void *obj)
        return str.data;
 }
 
+/*
+ * nodeToStringWithoutLocations -
+ *        returns the ascii representation of the Node as a palloc'd string
+ *
+ * This form prints location fields with their default value (not the actual
+ * field value).  This is useful for serializing node trees for storing into
+ * catalogs, where the location information is not useful since the original
+ * query string is not preserved anyway.
+ */
+char *
+nodeToStringWithoutLocations(const void *obj)
+{
+       StringInfoData str;
+       bool            save_write_location_fields;
+
+       save_write_location_fields = write_location_fields;
+       write_location_fields = false;
+
+       initStringInfo(&str);
+       outNode(&str, obj);
+
+       write_location_fields = save_write_location_fields;
+
+       return str.data;
+}
+
 /*
  * bmsToString -
  *        returns the ascii representation of the Bitmapset as a palloc'd 
string
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 2969dd831b..3abe47af94 100644
--- a/src/include/nodes/nodes.h
+++ b/src/include/nodes/nodes.h
@@ -195,6 +195,7 @@ extern void outBitmapset(struct StringInfoData *str,
 extern void outDatum(struct StringInfoData *str, uintptr_t value,
                                         int typlen, bool typbyval);
 extern char *nodeToString(const void *obj);
+extern char *nodeToStringWithoutLocations(const void *obj);
 extern char *bmsToString(const struct Bitmapset *bms);
 
 /*
-- 
2.44.0

Reply via email to