Thread: Useless field ispartitioned in CreateStmtContext
Hi!
When looking at the partition-related code, I found that the ispartitioned
field in CreateStmtContext is not used. It looks like we can safely remove it and
avoid invalid assignment logic.
Here's a very simple fix, any suggestion?
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 1e15ce10b48..6dea85cc2dc 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -89,7 +89,6 @@ typedef struct
List *alist; /* "after list" of things to do after creating
* the table */
IndexStmt *pkey; /* PRIMARY KEY index, if any */
- bool ispartitioned; /* true if table is partitioned */
PartitionBoundSpec *partbound; /* transformed FOR VALUES */
bool ofType; /* true if statement contains OF typename */
} CreateStmtContext;
@@ -246,7 +245,6 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
cxt.blist = NIL;
cxt.alist = NIL;
cxt.pkey = NULL;
- cxt.ispartitioned = stmt->partspec != NULL;
cxt.partbound = stmt->partbound;
cxt.ofType = (stmt->ofTypename != NULL);
@@ -3401,7 +3399,6 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
cxt.blist = NIL;
cxt.alist = NIL;
cxt.pkey = NULL;
- cxt.ispartitioned = (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
cxt.partbound = NULL;
cxt.ofType = false;
--
Best regards,
hugozhang
Hi!
@font-face {font-family:"Cambria Math"; panose-1:2 4 5 3 5 4 6 3 2 4;}@font-face {font-family:DengXian; panose-1:2 1 6 0 3 1 1 1 1 1;}@font-face {font-family:"\@等线"; panose-1:2 1 6 0 3 1 1 1 1 1;}p.MsoNormal, li.MsoNormal, div.MsoNormal {margin:0cm; font-size:11.0pt; font-family:DengXian; mso-ligatures:standardcontextual;}span.EmailStyle17 {mso-style-type:personal-compose; font-family:DengXian; color:windowtext;}.MsoChpDefault {mso-style-type:export-only;}div.WordSection1 {page:WordSection1;} Hi!
When looking at the partition-related code, I found that the ispartitioned
field in CreateStmtContext is not used. It looks like we can safely remove it and
avoid invalid assignment logic.
Here's a very simple fix, any suggestion?
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 1e15ce10b48..6dea85cc2dc 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -89,7 +89,6 @@ typedef struct
List *alist; /* "after list" of things to do after creating
* the table */
IndexStmt *pkey; /* PRIMARY KEY index, if any */
- bool ispartitioned; /* true if table is partitioned */
PartitionBoundSpec *partbound; /* transformed FOR VALUES */
bool ofType; /* true if statement contains OF typename */
} CreateStmtContext;
@@ -246,7 +245,6 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
cxt.blist = NIL;
cxt.alist = NIL;
cxt.pkey = NULL;
- cxt.ispartitioned = stmt->partspec != NULL;
cxt.partbound = stmt->partbound;
cxt.ofType = (stmt->ofTypename != NULL);
@@ -3401,7 +3399,6 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
cxt.blist = NIL;
cxt.alist = NIL;
cxt.pkey = NULL;
- cxt.ispartitioned = (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
cxt.partbound = NULL;
cxt.ofType = false;
I absolutely agree with you. I found that ispartitioned parameter has been defined but it is never used during optimization.
I also noticed that its local copy is being redefined in the ATExecDropIdentity, ATExecSetIdentity and ATExecAddIdentity functions.
So, I'm willing to agree with you.
BTW, the regression tests were successful.
-- Regards, Alena Rybakina Postgres Professional
On Thu, 24 Oct 2024 at 19:23, Alena Rybakina <a.rybakina@postgrespro.ru> wrote: > > Hi! > > On 24.10.2024 16:07, hugo wrote: > > Hi! > > When looking at the partition-related code, I found that the ispartitioned > > field in CreateStmtContext is not used. It looks like we can safely remove it and > > avoid invalid assignment logic. > > > > Here's a very simple fix, any suggestion? > > > > diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c > > index 1e15ce10b48..6dea85cc2dc 100644 > > --- a/src/backend/parser/parse_utilcmd.c > > +++ b/src/backend/parser/parse_utilcmd.c > > @@ -89,7 +89,6 @@ typedef struct > > List *alist; /* "after list" of things to do after creating > > * the table */ > > IndexStmt *pkey; /* PRIMARY KEY index, if any */ > > - bool ispartitioned; /* true if table is partitioned */ > > PartitionBoundSpec *partbound; /* transformed FOR VALUES */ > > bool ofType; /* true if statement contains OF typename */ > > } CreateStmtContext; > > @@ -246,7 +245,6 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString) > > cxt.blist = NIL; > > cxt.alist = NIL; > > cxt.pkey = NULL; > > - cxt.ispartitioned = stmt->partspec != NULL; > > cxt.partbound = stmt->partbound; > > cxt.ofType = (stmt->ofTypename != NULL); > > @@ -3401,7 +3399,6 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt, > > cxt.blist = NIL; > > cxt.alist = NIL; > > cxt.pkey = NULL; > > - cxt.ispartitioned = (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE); > > cxt.partbound = NULL; > > cxt.ofType = false; > > > > I absolutely agree with you. I found that ispartitioned parameter has been defined but it is never used during optimization. > > I also noticed that its local copy is being redefined in the ATExecDropIdentity, ATExecSetIdentity and ATExecAddIdentityfunctions. > > So, I'm willing to agree with you. > > BTW, the regression tests were successful. > > -- > Regards, > Alena Rybakina > Postgres Professional Hi all. This field was introduced by f0e4475 It was useful for various checks [1] but after we allowed unique keys on partition tables in commit eb7ed3f [2], this field became unneeded. CreateStmtContext is internal parser struct, so no not used outside PG core (i mean, by any extension). So, my suggestion is to remove it. Hugo, can you please create a CF entry for this patch? Patch itself looks good. [1] https://github.com/postgres/postgres/blob/f0e44751d7175fa3394da2c8f85e3ceb3cdbfe63/src/backend/parser/parse_utilcmd.c#L617 [2] https://github.com/postgres/postgres/commit/eb7ed3f3063401496e4aa4bd68fa33f0be31a72f#diff-5bd59ecc8991bacaefd56f7fe986287b8d664e62566eb3588c3845d7625cacf1L715 -- Best regards, Kirill Reshke
Hi!
When looking at the partition-related code, I found that the ispartitioned
field in CreateStmtContext is not used. It looks like we can safely remove it and
avoid invalid assignment logic.
Here's a very simple fix, any suggestion?
diff --git a/src/backend/parser/parse_utilcmd.c b/src/backend/parser/parse_utilcmd.c
index 1e15ce10b48..6dea85cc2dc 100644
--- a/src/backend/parser/parse_utilcmd.c
+++ b/src/backend/parser/parse_utilcmd.c
@@ -89,7 +89,6 @@ typedef struct
List *alist; /* "after list" of things to do after creating
* the table */
IndexStmt *pkey; /* PRIMARY KEY index, if any */
- bool ispartitioned; /* true if table is partitioned */
PartitionBoundSpec *partbound; /* transformed FOR VALUES */
bool ofType; /* true if statement contains OF typename */
} CreateStmtContext;
@@ -246,7 +245,6 @@ transformCreateStmt(CreateStmt *stmt, const char *queryString)
cxt.blist = NIL;
cxt.alist = NIL;
cxt.pkey = NULL;
- cxt.ispartitioned = stmt->partspec != NULL;
cxt.partbound = stmt->partbound;
cxt.ofType = (stmt->ofTypename != NULL);
@@ -3401,7 +3399,6 @@ transformAlterTableStmt(Oid relid, AlterTableStmt *stmt,
cxt.blist = NIL;
cxt.alist = NIL;
cxt.pkey = NULL;
- cxt.ispartitioned = (rel->rd_rel->relkind == RELKIND_PARTITIONED_TABLE);
cxt.partbound = NULL;
cxt.ofType = false;
--
Best regards,
hugozhang