Useless field ispartitioned in CreateStmtContext

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 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 ATExecAddIdentity functions.

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]https://github.com/postgres/postgres/blob/f0e44751d7175fa3394da2c8f85e3ceb3cdbfe63/src/backend/parser/parse_utilcmd.c#L617
but after we allowed unique keys on partition tables in commit eb7ed3f
[2]: https://github.com/postgres/postgres/commit/eb7ed3f3063401496e4aa4bd68fa33f0be31a72f#diff-5bd59ecc8991bacaefd56f7fe986287b8d664e62566eb3588c3845d7625cacf1L715
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

On Thu, 24 Oct 2024, 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

I don't see any cf entry for this patch, so I created one[1]https://commitfest.postgresql.org/50/5340/. I added Alena
and myself as reviewers. I can't find your account on cf, please add
yourself as author

[1]: https://commitfest.postgresql.org/50/5340/

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

Hi, Kirill

Sorry for the late reply, thanks for your suggestion.
A simple fix has been added to the attached patch.

--
hugo

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]/messages/by-id/202411051209.hzs5jktf6e3s@alvherre.pgsql

Patch status is now Rejected.

[0]: /messages/by-id/202411051209.hzs5jktf6e3s@alvherre.pgsql

--
Best regards,
Kirill Reshke