On 19.03.24 17:13, Peter Eisentraut wrote:
On 11.03.24 21:52, Matthias van de Meent wrote:
* 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.
I've gone with ParseLoc in the attached v8 patchset.
I have committed this one.
Next, I was looking at
v8-0003-pg_node_tree-Don-t-store-query-text-locations-in-.patch. After
applying that, I was looking how many uses of nodeToString() (with
locations) were left. I think your patch forgot to convert a number of
them, and there also might have been a few new ones that came in with
other recent patches. Might be hard to make sure all new developments
do this right. Plus, there are various mentions in the documentation
that should be updated. After considering all that, there weren't
really many callers of nodeToString() left. It's really only debugging
support in postgres.c and print.c, and a few places were it doesn't
matter, like the few places where it initializes "cooked expressions",
which were in turn already stripped of location fields at some earlier time.
So anyway, my idea was that we should turn this around and make
nodeToString() always drop location information, and instead add
nodeToStringWithLocations() for the few debugging uses. And this would
also be nice because then it matches exactly with the existing
stringToNodeWithLocations().
Attached patch shows this.
From 009c7359a5f0bf8c1ea9d685ec92f73a8dc4a345 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut <[email protected]>
Date: Wed, 20 Mar 2024 12:33:35 +0100
Subject: [PATCH] Do not output actual value of location fields in node
serialization by default
This changes nodeToString() to not output the actual value of location
fields in nodes, but instead it writes -1. This mirrors the fact that
stringToNode() also does not read location field values but always
stores -1.
For most uses of nodeToString(), which is to store nodes in catalog
fields, this is more useful. We don't store original query texts in
catalogs, so any lingering query location values are not meaningful.
For debugging purposes, there is a new nodeToStringWithLocations(),
which mirrors the existing stringToNodeWithLocations(). This is used
for WRITE_READ_PARSE_PLAN_TREES and nodes/print.c functions, which
covers all the debugging uses.
---
src/backend/nodes/outfuncs.c | 37 +++++++++++++++++++++++++++++++++---
src/backend/nodes/print.c | 6 +++---
src/backend/tcop/postgres.c | 6 +++---
src/include/nodes/nodes.h | 1 +
4 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/src/backend/nodes/outfuncs.c b/src/backend/nodes/outfuncs.c
index 29cbc83bd9f..b25ca952b45 100644
--- a/src/backend/nodes/outfuncs.c
+++ b/src/backend/nodes/outfuncs.c
@@ -25,6 +25,9 @@
#include "nodes/pg_list.h"
#include "utils/datum.h"
+/* State flag that determines how nodeToStringInternal() should treat location
fields */
+static bool write_location_fields = false;
+
static void outChar(StringInfo str, char c);
static void outDouble(StringInfo str, double d);
@@ -88,7 +91,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) \
@@ -758,18 +761,46 @@ outNode(StringInfo str, const void *obj)
/*
* nodeToString -
* returns the ascii representation of the Node as a palloc'd string
+ *
+ * write_loc_fields determines whether location fields are output with their
+ * actual value rather than -1. The actual value can be useful for debugging,
+ * but for most uses, the actual value is not useful, since the original query
+ * string is no longer available.
*/
-char *
-nodeToString(const void *obj)
+static char *
+nodeToStringInternal(const void *obj, bool write_loc_fields)
{
StringInfoData str;
+ bool save_write_location_fields;
+
+ save_write_location_fields = write_location_fields;
+ write_location_fields = write_loc_fields;
/* see stringinfo.h for an explanation of this maneuver */
initStringInfo(&str);
outNode(&str, obj);
+
+ write_location_fields = save_write_location_fields;
+
return str.data;
}
+/*
+ * Externally visible entry points
+ */
+char *
+nodeToString(const void *obj)
+{
+ return nodeToStringInternal(obj, false);
+}
+
+char *
+nodeToStringWithLocations(const void *obj)
+{
+ return nodeToStringInternal(obj, true);
+}
+
+
/*
* bmsToString -
* returns the ascii representation of the Bitmapset as a palloc'd
string
diff --git a/src/backend/nodes/print.c b/src/backend/nodes/print.c
index d2a58a5956a..02798f4482d 100644
--- a/src/backend/nodes/print.c
+++ b/src/backend/nodes/print.c
@@ -38,7 +38,7 @@ print(const void *obj)
char *s;
char *f;
- s = nodeToString(obj);
+ s = nodeToStringWithLocations(obj);
f = format_node_dump(s);
pfree(s);
printf("%s\n", f);
@@ -56,7 +56,7 @@ pprint(const void *obj)
char *s;
char *f;
- s = nodeToString(obj);
+ s = nodeToStringWithLocations(obj);
f = pretty_format_node_dump(s);
pfree(s);
printf("%s\n", f);
@@ -74,7 +74,7 @@ elog_node_display(int lev, const char *title, const void
*obj, bool pretty)
char *s;
char *f;
- s = nodeToString(obj);
+ s = nodeToStringWithLocations(obj);
if (pretty)
f = pretty_format_node_dump(s);
else
diff --git a/src/backend/tcop/postgres.c b/src/backend/tcop/postgres.c
index aec1b194424..22577390c44 100644
--- a/src/backend/tcop/postgres.c
+++ b/src/backend/tcop/postgres.c
@@ -640,7 +640,7 @@ pg_parse_query(const char *query_string)
*/
#ifdef WRITE_READ_PARSE_PLAN_TREES
{
- char *str = nodeToString(raw_parsetree_list);
+ char *str = nodeToStringWithLocations(raw_parsetree_list);
List *new_list = stringToNodeWithLocations(str);
pfree(str);
@@ -848,7 +848,7 @@ pg_rewrite_query(Query *query)
foreach(lc, querytree_list)
{
Query *curr_query = lfirst_node(Query, lc);
- char *str = nodeToString(curr_query);
+ char *str = nodeToStringWithLocations(curr_query);
Query *new_query = stringToNodeWithLocations(str);
/*
@@ -930,7 +930,7 @@ pg_plan_query(Query *querytree, const char *query_string,
int cursorOptions,
char *str;
PlannedStmt *new_plan;
- str = nodeToString(plan);
+ str = nodeToStringWithLocations(plan);
new_plan = stringToNodeWithLocations(str);
pfree(str);
diff --git a/src/include/nodes/nodes.h b/src/include/nodes/nodes.h
index 2969dd831b2..c52788abe3e 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 *nodeToStringWithLocations(const void *obj);
extern char *bmsToString(const struct Bitmapset *bms);
/*
--
2.44.0
