public inbox for pgsql-general@postgresql.org
help / color / mirror / Atom feedFrom: Yu Shi (Fujitsu) <shiy.fnst@fujitsu.com>
To: shveta malik <shveta.malik@gmail.com>
To: vignesh C <vignesh21@gmail.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>
Cc: Zhijie Hou (Fujitsu) <houzj.fnst@fujitsu.com>
Cc: Ajin Cherian <itsajin@gmail.com>
Cc: Wei Wang (Fujitsu) <wangw.fnst@fujitsu.com>
Cc: Runqi Tian <runqidev@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>
Cc: Tom Lane <tgl@sss.pgh.pa.us>
Cc: li jie <ggysxcq@gmail.com>
Cc: Dilip Kumar <dilipbalaut@gmail.com>
Cc: Alvaro Herrera <alvherre@alvh.no-ip.org>
Cc: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Japin Li <japinli@hotmail.com>
Cc: rajesh singarapu <rajesh.rs0541@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Zheng Li <zhengli10@gmail.com>
Subject: RE: Support logical replication of DDLs
Date: Mon, 29 May 2023 06:15:28 +0000
Message-ID: <OSZPR01MB63102C42A24D59FACF6D9CD6FD4A9@OSZPR01MB6310.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <CAJpy0uCGnRQnpNVQaApRxqN=9t5VpHRk5b+Mfdf3cqCEW=s+4A@mail.gmail.com>
References: <CALDaNm3zSxK1g0UTo4RjPR3JEc-vyPzANOnKEKjiW88xtUUdzA@mail.gmail.com>
<CAJpy0uCGnRQnpNVQaApRxqN=9t5VpHRk5b+Mfdf3cqCEW=s+4A@mail.gmail.com>
On Mon, May 22, 2023 1:57 PM shveta malik <shveta.malik@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
reply
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-general@postgresql.org
Cc: shiy.fnst@fujitsu.com, shveta.malik@gmail.com, vignesh21@gmail.com, amit.kapila16@gmail.com, houzj.fnst@fujitsu.com, itsajin@gmail.com, wangw.fnst@fujitsu.com, runqidev@gmail.com, smithpb2250@gmail.com, tgl@sss.pgh.pa.us, ggysxcq@gmail.com, dilipbalaut@gmail.com, alvherre@alvh.no-ip.org, sawada.mshk@gmail.com, japinli@hotmail.com, rajesh.rs0541@gmail.com, pgsql-hackers@lists.postgresql.org, zhengli10@gmail.com
Subject: RE: Support logical replication of DDLs
In-Reply-To: <OSZPR01MB63102C42A24D59FACF6D9CD6FD4A9@OSZPR01MB6310.jpnprd01.prod.outlook.com>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox