Thread: Don't treate IndexStmt like AlterTable when DefineIndex is called from ProcessUtilitySlow.
Don't treate IndexStmt like AlterTable when DefineIndex is called from ProcessUtilitySlow.
From
正华吕
Date:
Hi,
Recently read the code, I find that when calling DefineIndex
from ProcessUtilitySlow, is_alter_table will be set true if
this statement is came from expandTableLikeClause.
I check the code of DefineIndex, there are only two places use
is_alter_table:
1. the function index_check_primary_key
2. print a debug log on what the statement is
For 1, since we are doing create table xxx (like yyy including
indexes), we are sure that the check relationHasPrimaryKey in the
function index_check_primary_key will be satisfied because we are just
create the new table.
For 2, I do not think it will mislead the user if we print it as
CreateStmt.
Based on the above, I think we can always a false value
for is_alter_table when DefineIndex is called from
ProcessUtilitySlow.
from ProcessUtilitySlow, is_alter_table will be set true if
this statement is came from expandTableLikeClause.
I check the code of DefineIndex, there are only two places use
is_alter_table:
1. the function index_check_primary_key
2. print a debug log on what the statement is
For 1, since we are doing create table xxx (like yyy including
indexes), we are sure that the check relationHasPrimaryKey in the
function index_check_primary_key will be satisfied because we are just
create the new table.
For 2, I do not think it will mislead the user if we print it as
CreateStmt.
Based on the above, I think we can always a false value
for is_alter_table when DefineIndex is called from
ProcessUtilitySlow.
Here I attach a patch. Any ideas?
Thanks a lot.
Best,
Zhenghua Lyu
Attachment
Re: Don't treate IndexStmt like AlterTable when DefineIndex is called from ProcessUtilitySlow.
From
Tom Lane
Date:
=?UTF-8?B?5q2j5Y2O5ZCV?= <kainwen@gmail.com> writes:
> Recently read the code, I find that when calling DefineIndex
> from ProcessUtilitySlow, is_alter_table will be set true if
> this statement is came from expandTableLikeClause.
Yeah.
> Based on the above, I think we can always a false value
> for is_alter_table when DefineIndex is called from
> ProcessUtilitySlow.
Why do you think this is an improvement? Even if it's correct,
the code savings is so negligible that I'm not sure I want to
expend brain cells on figuring out whether it's correct. The
comment you want to remove does not suggest that it's optional
which value we should pass, so I think the burden of proof
is to show that this patch is okay not that somebody else
has to demonstrate that it isn't.
regards, tom lane
Re: Don't treate IndexStmt like AlterTable when DefineIndex is called from ProcessUtilitySlow.
From
正华吕
Date:
Hi, thanks a lot for your reply!
> Why do you think this is an improvement?
I hit the issue in Greenplum Database (Massively Parallel Postgres),
the MPP architecture is that coordinator dispatch statement to segments.
The dispatch logic is quit different for AlterTable and CreateTableLike:
* alter table: for each sub command, it will not dispatch; later it will dispatch
the alter table statement as a whole.
* for create table like statement, like `create table t (like t1 including indexes);`
this statement's 2nd stmt, has to dispatch to segments, but now it is treated
as alter table, the dispatch logic is broken for this case in Greenplum.
I look into the issue and Greenplum Database wants to keep align with Upstream
as possible. That is why I ask if we can force it to false.
Best,
Zhenghua Lyu
Tom Lane <tgl@sss.pgh.pa.us> 于2022年11月18日周五 06:26写道：
正华吕 <kainwen@gmail.com> writes:
> Recently read the code, I find that when calling DefineIndex
> from ProcessUtilitySlow, is_alter_table will be set true if
> this statement is came from expandTableLikeClause.
Yeah.
> Based on the above, I think we can always a false value
> for is_alter_table when DefineIndex is called from
> ProcessUtilitySlow.
Why do you think this is an improvement? Even if it's correct,
the code savings is so negligible that I'm not sure I want to
expend brain cells on figuring out whether it's correct. The
comment you want to remove does not suggest that it's optional
which value we should pass, so I think the burden of proof
is to show that this patch is okay not that somebody else
has to demonstrate that it isn't.
regards, tom lane