Re: About adding a new filed to a struct in primnodes.h

On Wed, Nov 25, 2020 at 8:10 AM Andy Fan <zhihui(dot)fan1213(at)gmail(dot)com> wrote:

>
>
> On Tue, Nov 24, 2020 at 11:11 PM Alvaro Herrera <alvherre(at)alvh(dot)no-ip(dot)org>
> wrote:
>
>> On 2020-Nov-24, Andy Fan wrote:
>>
>> > then we modified the copy/read/out functions for this node. In
>> > _readFuncExpr,
>> > we probably add something like
>>
>> > [ ... ]
>>
>> > Then we will get a compatible issue if we create a view with the node in
>> > the older version and access the view with the new binary.
>>
>> When nodes are modified, you have to increment CATALOG_VERSION_NO which
>> makes the new code incompatible with a datadir previously created
>
>
> Thank you Alvaro. I just think this issue can be avoided without causing
> the incompatible issue. IIUC, after the new binary isn't compatible with
> the datadir, the user has to dump/restore the whole database or use
> pg_upgrade. It is kind of extra work.
>
>
> -- for precisely this reason.
>>
>
> I probably didn't get the real point of this, sorry about that.
>
> --
> Best Regards
> Andy Fan
>

What I mean here is something like below.

diff --git a/src/backend/nodes/read.c b/src/backend/nodes/read.c
index 8c1e39044c..c3eba00639 100644
--- a/src/backend/nodes/read.c
+++ b/src/backend/nodes/read.c
@@ -29,6 +29,7 @@

/* Static state for pg_strtok */
static const char *pg_strtok_ptr = NULL;
+static const char *pg_strtok_extend(int *length, bool testonly);

/* State flag that determines how readfuncs.c should treat location fields
*/
#ifdef WRITE_READ_PARSE_PLAN_TREES
@@ -102,6 +103,20 @@ stringToNodeWithLocations(const char *str)
#endif

+const char*
+pg_strtok(int *length)
+{
+ return pg_strtok_extend(length, false);
+}
+
+/*
+ * Just peak the next filed name without changing the global state.
+ */
+const char*
+pg_peak_next_field(int *length)
+{
+ return pg_strtok_extend(length, true);
+}
/*****************************************************************************
*
* the lisp token parser
@@ -149,7 +164,7 @@ stringToNodeWithLocations(const char *str)
* as a single token.
*/
const char *
-pg_strtok(int *length)
+pg_strtok_extend(int *length, bool testonly)
{
const char *local_str; /* working pointer to string */
const char *ret_str; /* start of token to return */
@@ -162,7 +177,8 @@ pg_strtok(int *length)
if (*local_str == '\0')
{
*length = 0;
- pg_strtok_ptr = local_str;
+ if (!testonly)
+ pg_strtok_ptr = local_str;
return NULL; /* no more tokens */
}

@@ -199,7 +215,8 @@ pg_strtok(int *length)
if (*length == 2 && ret_str[0] == '<' && ret_str[1] == '>')
*length = 0;

- pg_strtok_ptr = local_str;
+ if (!testonly)
+ pg_strtok_ptr = local_str;

return ret_str;
}

-- the below is a demo code to use it.
diff --git a/src/backend/nodes/readfuncs.c b/src/backend/nodes/readfuncs.c
index 76ab5ae8b7..c19cd45793 100644
--- a/src/backend/nodes/readfuncs.c
+++ b/src/backend/nodes/readfuncs.c
@@ -689,13 +689,27 @@ _readFuncExpr(void)

READ_OID_FIELD(funcid);
READ_OID_FIELD(funcresulttype);
- READ_BOOL_FIELD(funcretset);
+ token = pg_peak_next_field(&length);
+ if (memcmp(token, ":funcretset", strlen(":funcretset")) == 0)
+ {
+ READ_BOOL_FIELD(funcretset);
+ }
+ else
+ local_node->funcretset = false;
+
READ_BOOL_FIELD(funcvariadic);
READ_ENUM_FIELD(funcformat, CoercionForm);
- READ_OID_FIELD(funccollid);
+ READ_OID_FIELD(funccollid);
READ_OID_FIELD(inputcollid);
READ_NODE_FIELD(args);
- READ_LOCATION_FIELD(location);
+
+ token = pg_peak_next_field(&length);
+ if (memcmp(token, ":location", strlen(":location")) == 0)
+ {
+ READ_LOCATION_FIELD(location);
+ }
+ else
+ local_node->location = -1;

READ_DONE();
}

After writing it, I am feeling that this will waste a bit of performance
since we need to token a part of the string twice. But looks the overhead
looks good to me and can be avoided if we refactor the code again with
"read_fieldname_or_nomove(char *fieldname, int *length) " function.

--
Best Regards
Andy Fan