Thread: Useless field ispartitioned in CreateStmtContext

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

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