MIME-Version: 1.0
References: 
 <CAAD30ULCxqOJp0sffm_y9jNC4BVPYv7Q7_va_JE8qyfRXkfu+g@mail.gmail.com>
 <CAFPTHDaaewvYUznZD1YjUQnvHycgvgMKNK4w=V8Q-8MKTpVDrw@mail.gmail.com>
 <OS3PR01MB6275025C397CC22E446B1F589EBC9@OS3PR01MB6275.jpnprd01.prod.outlook.com>
 <OS0PR01MB5716765B2D38786DA3943E0294809@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <CAFPTHDbAJPccMcZnOraBy14hM6qBJxYqcRwK6iV6=gtL6VZQwQ@mail.gmail.com>
 <CALDaNm3NUO8ofK64N7HMtNmUP=52R8_jWzrekqAm7m7wqZjwaQ@mail.gmail.com>
 <CALDaNm3XUKfD+nD1AVvSuZyUY_zRk_eyz+Pt9t13N8WXViR6pw@mail.gmail.com>
 <3032112.1679865718@sss.pgh.pa.us>
 <CAA4eK1K3VXfTWXbLADcH81J==7ussvNdqLFHN68sEokDPueu7w@mail.gmail.com>
 <CAA4eK1++Y7a2SQq55DXT6neghZgj3j+pQ74=8zfT3k8Tkdj0ZA@mail.gmail.com>
 <OS0PR01MB571638B29E37EB9A48507DAE94889@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB571636F0F1307AB1A868B06A948E9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
 <OS0PR01MB57169AB2355A90C9FA87DE75948E9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
In-Reply-To: 
 <OS0PR01MB57169AB2355A90C9FA87DE75948E9@OS0PR01MB5716.jpnprd01.prod.outlook.com>
From: vignesh C <vignesh21@gmail.com>
Date: Thu, 30 Mar 2023 15:15:59 +0530
Message-ID: 
 <CALDaNm2vBN8oMv-7G=DH5rR-u40JGbR9aP4B6nwr71qw17rPFA@mail.gmail.com>
Subject: Re: Support logical replication of DDLs
To: "houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, Ajin Cherian <itsajin@gmail.com>,
	"wangw.fnst@fujitsu.com" <wangw.fnst@fujitsu.com>,
 Runqi Tian <runqidev@gmail.com>,
	Peter Smith <smithpb2250@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>,
 li jie <ggysxcq@gmail.com>,
	Dilip Kumar <dilipbalaut@gmail.com>,
 Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Masahiko Sawada <sawada.mshk@gmail.com>, Japin Li <japinli@hotmail.com>,
	rajesh singarapu <rajesh.rs0541@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Zheng Li <zhengli10@gmail.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CALDaNm2vBN8oMv-7G%3DDH5rR-u40JGbR9aP4B6nwr71qw17rPFA%40mail.gmail.com>
Precedence: bulk

On Thu, 30 Mar 2023 at 13:29, houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
>
>
> > -----Original Message-----
> > From: houzj.fnst@fujitsu.com <houzj.fnst@fujitsu.com>
> > Sent: Thursday, March 30, 2023 2:37 PM
> >
> > On Tuesday, March 28, 2023 12:13 PM houzj.fnst@fujitsu.com
> > <houzj.fnst@fujitsu.com> wrote:
> > >
> > > On Monday, March 27, 2023 8:08 PM Amit Kapila
> > <amit.kapila16@gmail.com>
> > > wrote:
> > > > On Mon, Mar 27, 2023 at 12:07=E2=80=AFPM Amit Kapila <amit.kapila16=
@gmail.com>
> > > > wrote:
> > > > >
> > > > > On Mon, Mar 27, 2023 at 2:52=E2=80=AFAM Tom Lane <tgl@sss.pgh.pa.=
us> wrote:
> > > > > >
> > > > >
> > > > > > I suggest taking a couple of steps back from the minutiae of th=
e
> > > > > > patch, and spending some hard effort thinking about how the thi=
ng
> > > > > > would be controlled in a useful fashion (that is, a real design
> > > > > > for the filtering that was mentioned at the very outset), and
> > > > > > about the security issues, and about how we could get to a
> > committable
> > > patch.
> > > > > >
> > > > >
> > > > > Agreed. I'll try to summarize the discussion we have till now on
> > > > > this and share my thoughts on the same in a separate email.
> > > > >
> > > >
> > > > The idea to control what could be replicated is to introduce a new
> > > > publication option 'ddl' along with current options 'publish' and
> > > > 'publish_via_partition_root'. The values of this new option could b=
e
> > > > 'table', 'function', 'all', etc. Here 'all' enables the replication=
 of
> > > > all supported DDL commands. Example usage for this would be:
> > > > Example:
> > > > Create a new publication with all ddl replication enabled:
> > > >   CREATE PUBLICATION pub1 FOR ALL TABLES with (ddl =3D 'all');
> > > >
> > > > Enable table ddl replication for an existing Publication:
> > > >   ALTER PUBLICATION pub2 SET (ddl =3D 'table');
> > > >
> > > > This is what seems to have been discussed but I think we can even
> > > > extend it to support based on operations/commands, say one would li=
ke
> > > > to publish only 'create' and 'drop' of tables. Then we can extend t=
he
> > > > existing publish option to have values like 'create', 'alter', and =
'drop'.
> > > >
> > > > Another thing we are considering related to this is at what level
> > > > these additional options should be specified. We have three variant=
s
> > > > FOR TABLE, FOR ALL TABLES, and FOR TABLES IN SCHEMA that enables
> > > > replication. Now, for the sake of simplicity, this new option is
> > > > discussed to be provided only with FOR ALL TABLES variant but I thi=
nk
> > > > we can provide it with other variants with some additional
> > > > restrictions like with FOR TABLE, we can only specify 'alter' and
> > > > 'drop' for publish option. Now, though possible, it brings addition=
al
> > > > complexity to support it with variants other than FOR ALL TABLES
> > > > because then we need to ensure additional filtering and possible
> > > > modification of the content we have to send to downstream. So, we c=
an
> > even
> > > decide to first support it only FOR ALL TABLES variant.
> > > >
> > > > The other point to consider for publish option 'ddl =3D table' is
> > > > whether we need to allow replicating dependent objects like say som=
e
> > > > user-defined type is used in the table. I guess the difficulty here
> > > > would be to identify which dependents we want to allow.
> > > >
> > > > I think in the first version we should allow to replicate only some=
 of
> > > > the objects instead of everything. For example, can we consider onl=
y
> > > > allowing tables and indexes in the first version? Then extend it in=
 a phased
> > > manner?
> > >
> > > I think supporting table related stuff in the first version makes sen=
se and the
> > > patch size could be reduced to a suitable size.
> >
> > Based on the discussion, I split the patch into four parts: Table DDL
> > replication(0001,0002), Index DDL replication(0003), ownership stuff fo=
r table
> > and index(0004), other DDL's replication(0005).
> >
> > In this version, I mainly tried to split the patch set, and there are f=
ew
> > OPEN items we need to address later:
> >
> > 1) The current publication "ddl" option only have two values: table, al=
l. We
> >    also need to add index and maybe other objects in the list.
> >
> > 2) Need to improve the syntax stuff. Currently, we store the option val=
ue of
> >    the "with (ddl =3D xx)" via different columns in the catalog, the
> >    catalog(pg_publication) will have more and more columns as we add
> > support
> >    for logical replication of other objects in the future. We could sto=
re it as
> >    an text array instead.
> >
> >    OTOH, since we have proposed some other more flexible syntax to -hac=
kers,
> > the current
> >    syntax might be changed which might also solve this problem.
> >
> > 3) The test_ddl_deparse_regress test module is not included in the set,
> > because
> >    I think we also need to split it into table stuff, index stuff and o=
thers,
> >    we can share it after finishing that.
> >
> > 4) The patch set could be spitted further to make it easier for reviewe=
r like:
> >    infrastructure for deparser, deparser, logical-decoding, built-in lo=
gical
> >    replication, We can do it after some analysis.
> >
> > [1]
> > https://www.postgresql.org/message-id/OS0PR01MB571646874A3E165D939
> > 99A9D94889%40OS0PR01MB5716.jpnprd01.prod.outlook.com
>
> The patch needs a rebase due to a recent commit da324d6, here is the reba=
sed version.

Thanks for the patches, Few comments:
1: create unlogged is replicated but the insert on the same is not replicat=
ed:
create unlogged table t3(c1 int); -- The insert on this is not replicated

2: "Using index tablespace" option is not replicated:
create table t3 (c1 int unique using index tablespace tbs1);

publisher:
\d+ t3
                                           Table "public.t3"
 Column |  Type   | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------=
+--------------+-------------
 c1     | integer |           |          |         | plain   |
    |              |
Indexes:
    "t3_c1_key" UNIQUE CONSTRAINT, btree (c1), tablespace "tbs1"
Publications:
    "pub1"
Access method: heap

Subscriber:
\d+ t3
                                           Table "public.t3"
 Column |  Type   | Collation | Nullable | Default | Storage |
Compression | Stats target | Description
--------+---------+-----------+----------+---------+---------+-------------=
+--------------+-------------
 c1     | integer |           |          |         | plain   |
    |              |
Indexes:
    "t3_c1_key" UNIQUE CONSTRAINT, btree (c1)
Access method: heap

3:The create table is not replicated whereas the drop table of the
same is replicated with PUBLISH_VIA_PARTITION_ROOT as default (false):
create table t1(c1 int) partition by hash ( c1);
drop table t1;

4: Should we document tablespace creation should be taken care of by user:
create table t1(c1 int) tablespace tbs1; -- As tablespace tbs1 does
not exist in the subscriber, should we add some documentation for
this.

5: temporary table is not replicated, should we add some documentation for =
this:
create global temporary table t2(c1 int); -- Should we add some
documentation for this

Regards,
Vignesh