On Mon, May 22, 2023 1:57 PM shveta malik <shveta.ma...@gmail.com> wrote:
>
> Please find the new set of patches for object-tree Removal. The new
> changes are in patch 0008 only. The new changes address object tree
> removal for below commands.
>
> create sequence
> alter sequence
> alter object owner to
> alter object set schema
> alter object rename
>
> In this patch 0008, ddldeparse.c is now object-tree free for all the
> table related commands. Index related commands are yet to be done.
>
Thanks for updating the patch. Here are some comments.
0001 patch
-----
1.
+ colname = get_attname(ownerId, depform->refobjsubid, false);
+ if (colname == NULL)
+ continue;
missing_ok is false when calling get_attname(), so is there any case that
colname is NULL?
2.
+ case AT_SetStatistics:
+ {
+ Assert(IsA(subcmd->def, Integer));
+ if (subcmd->name)
+ tmp_obj = new_objtree_VA("ALTER
COLUMN %{column}I SET STATISTICS %{statistics}n", 3,
+
"type", ObjTypeString, "set statistics",
+
"column", ObjTypeString, subcmd->name,
+
"statistics", ObjTypeInteger,
+
intVal((Integer *) subcmd->def));
+ else
+ tmp_obj = new_objtree_VA("ALTER
COLUMN %{column}n SET STATISTICS %{statistics}n", 3,
+
"type", ObjTypeString, "set statistics",
+
"column", ObjTypeInteger, subcmd->num,
+
"statistics", ObjTypeInteger,
+
intVal((Integer *) subcmd->def));
+ subcmds = lappend(subcmds,
new_object_object(tmp_obj));
+ }
+ break;
I think subcmd->name will be NULL only if relation type is index. So should it
be removed because currently only table commands are supported?
0002 patch
-----
3.
+ /* Skip adding constraint for inherits
table sub command */
+ if (!constrOid)
+ continue;
Would it be better to use OidIsValid() here?
0008 patch
-----
4.
case AT_AddColumn:
/* XXX need to set the "recurse" bit somewhere?
*/
Assert(IsA(subcmd->def, ColumnDef));
- tree = deparse_ColumnDef(rel, dpcontext, false,
-
(ColumnDef *) subcmd->def, true, &expr);
mark_function_volatile(context, expr);
After this change, `expr` is not assigned a value when mark_function_volatile
is called.
Some problems I saw :
-----
5.
create table p1(f1 int);
create table p1_c1() inherits(p1);
alter table p1 add constraint inh_check_constraint1 check (f1 > 0);
alter table p1_c1 add constraint inh_check_constraint1 check (f1 > 0);
The re-formed command of the last command is "ALTER TABLE public.p1_c1", which
seems to be wrong.
6.
SET allow_in_place_tablespaces = true;
CREATE TABLESPACE ddl_tblspace LOCATION '';
RESET allow_in_place_tablespaces;
CREATE TABLE tbl_index_tblspe (a int, PRIMARY KEY(a) USING INDEX TABLESPACE
ddl_tblspace) ;
The re-formed command of the last command seems incorrect:
CREATE TABLE public.tbl_index_tblspe (a pg_catalog.int4 STORAGE PLAIN ,
USING INDEX TABLESPACE ddl_tblspace)
7.
CREATE TABLE part2_with_multiple_storage_params(
id int,
name varchar
) WITH (autovacuum_enabled);
re-formed command: CREATE TABLE public.part2_with_multiple_storage_params (id
pg_catalog.int4 STORAGE PLAIN , name pg_catalog."varchar" STORAGE EXTENDED
COLLATE pg_catalog."default" ) WITH (vacuum_index_cleanup = 'on',
autovacuum_vacuum_scale_factor = '0.2', vacuum_truncate = 'true',
autovacuum_enabled = 'TRUE')
When the option is not specified, re-formed command used uppercase letters. The
reloptions column in pg_class of the original command is
"{autovacuum_enabled=true}", but that of the re-formed command is
"{autovacuum_enabled=TRUE}". I tried to add this case to
test_ddl_deparse_regress test module but the test failed because the dumped
results are different.
Regards,
Shi Yu
