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
On Thu, 24 Oct 2024 at 18:08, hugo <2689496754@qq.com> 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; > > > > -- > > Best regards, > > hugozhang Hi, I noticed a change of cf entry, thanks! Can you please also send your patch as attachment? You need to do something like git format-patch -v1 -1 and attach resulting file -- Best regards, Kirill Reshke
On 2024-Nov-05, hugo wrote: > Hi, Kirill > > Sorry for the late reply, thanks for your suggestion. > A simple fix has been added to the attached patch. Actually, AFAICT my patch at https://commitfest.postgresql.org/50/5224/ adds a use of this field, so if you remove it, I might have to put it back for that. -- Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/ "But static content is just dynamic content that isn't moving!" http://smylers.hates-software.com/2007/08/15/fe244d0c.html
On Tue, 5 Nov 2024 at 16:51, hugo <2689496754@qq.com> wrote: > > Hi, Kirill > > Sorry for the late reply, thanks for your suggestion. > A simple fix has been added to the attached patch. > > -- > hugo > Hi! This field is actually used after 14e87ff. Just like Álvaro stated in [0] Patch status is now Rejected. [0] https://www.postgresql.org/message-id/202411051209.hzs5jktf6e3s@alvherre.pgsql -- Best regards, Kirill Reshke