Thread: About adding a new filed to a struct in primnodes.h
Hi:
struct FuncExpr
{
+ int newf;
};
then we modified the copy/read/out functions for this node. In _readFuncExpr,
we probably add something like
static FuncExpr
_readFuncExpr(..)
{
..
+ READ_INT_FILED(newf);
};
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. I think we can
have bypass this issue easily with something like
READ_INT_FIELD_UNMUST(newf, defaultvalue);
However I didn't see any code like this in our code base. does it doesn't
work or is it something not worth doing?
Best Regards
Andy Fan
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 -- for precisely this reason.
On Tue, Nov 24, 2020 at 11:11 PM Alvaro Herrera <alvherre@alvh.no-ip.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
On Wed, Nov 25, 2020 at 8:10 AM Andy Fan <zhihui.fan1213@gmail.com> wrote:
On Tue, Nov 24, 2020 at 11:11 PM Alvaro Herrera <alvherre@alvh.no-ip.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 createdThank you Alvaro. I just think this issue can be avoided without causingthe incompatible issue. IIUC, after the new binary isn't compatible withthe datadir, the user has to dump/restore the whole database or usepg_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 RegardsAndy 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;
}
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();
}
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
Andy Fan <zhihui.fan1213@gmail.com> writes:
> What I mean here is something like below.
What exactly would be the value of that?
There is work afoot, or at least on people's to-do lists, to mechanize
creation of the outfuncs/readfuncs/etc code directly from the Node
struct declarations. So special cases for particular fields are going
to be looked on with great disfavor, even if you can make a case that
there's some reason to do it. (Which I'm not seeing. We've certainly
never had to do it in the past.)
regards, tom lane
On Wed, Nov 25, 2020 at 9:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andy Fan <zhihui.fan1213@gmail.com> writes:
> What I mean here is something like below.
What exactly would be the value of that?
There is work afoot, or at least on people's to-do lists, to mechanize
creation of the outfuncs/readfuncs/etc code directly from the Node
struct declarations. So special cases for particular fields are going
to be looked on with great disfavor,
I agree with this, but I don't think there is no value in my suggestion
unless I missed something. Per my current understanding, the code
is too easy to make the datadir incompatible with binary, which requires
user to do some extra work and my suggestion is to avoid that and
it is the value. I think it is possible to make this strategy work with the
"mechanize creation of the outfuncs/readfuncs/ect node", but there is
no point to try it unless we make agreement on if we should do that.
even if you can make a case that
there's some reason to do it. (Which I'm not seeing. We've certainly
never had to do it in the past.)
regards, tom lane
Best Regards
Andy Fan
Andy Fan <zhihui.fan1213@gmail.com> writes:
> On Wed, Nov 25, 2020 at 9:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What exactly would be the value of that?
>> ...
> I agree with this, but I don't think there is no value in my suggestion
> unless I missed something. Per my current understanding, the code
> is too easy to make the datadir incompatible with binary, which requires
> user to do some extra work and my suggestion is to avoid that and
> it is the value.
It'd be fairly pointless to worry about this so far as ordinary users are
concerned, who are only going to encounter the situation in connection
with a major-version upgrade. There is no way that the only catalog
incompatibility they'd face is an addition or removal of a field in some
query node. In practice, a major-version upgrade is also going to
involve things like these:
* addition (or, sometimes, removal) of entire system catalogs
* addition, removal, or redefinition of columns within an existing catalog
* addition or removal of standard entries in system catalogs
* restructuring of stored rules/expressions in ways that are more
complicated than simple addition/removal of fields
The second of those, in particular, is quite fatal to any idea of
making a version-N backend executable work with version-not-N
catalogs. Catalog rowtypes are wired into the C code at a pretty
basic level.
We already sweat a great deal to make user table contents be upwards
compatible across major versions. I think that trying to take on
some similar guarantee with respect to system catalog contents would
serve mostly to waste a lot of developer manpower that can be put to
much better uses.
regards, tom lane
On Wed, Nov 25, 2020 at 11:54 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
Andy Fan <zhihui.fan1213@gmail.com> writes:
> On Wed, Nov 25, 2020 at 9:40 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What exactly would be the value of that?
>> ...
> I agree with this, but I don't think there is no value in my suggestion
> unless I missed something. Per my current understanding, the code
> is too easy to make the datadir incompatible with binary, which requires
> user to do some extra work and my suggestion is to avoid that and
> it is the value.
In practice, a major-version upgrade is also going to involve things like these:
Actually I thought if this would be the main reason for this case, now you
confirmed that. Thank you.
We already sweat a great deal to make user table contents be upwards
compatible across major versions. I think that trying to take on
some similar guarantee with respect to system catalog contents would
serve mostly to waste a lot of developer manpower that can be put to
much better uses.
Less experienced people know what to do, but real experienced people
know what not to do. Thanks for sharing the background in detail!
Actually I may struggle too much in a case where I port a feature with
node modification into a lower version which I have to consider the
compatible issue. The feature is inline-cte, We added a new field
in CommonTableExpr which is used to hint optimizer if to think about
the "inline" or not. From the porting aspect, the new field will cause
the incompatible issue, however a hint would not. I know porting will
not be a concern of the community, and I would not argue about hints
in this thread, I just share my little experience for reference here.
Thank you in all.
Best Regards
Andy Fan