MIME-Version: 1.0
References: 
 <CAAD30ULtoGp8L_GKbV15Wnm+X5r=SE7MOnYHuqBr396m26jJSA@mail.gmail.com>
 <202203162206.7spggyktx63e@alvherre.pgsql>
 <CAAD30UKRUusq8JyyHzAv71=ncN22OE8OkOOyAWvRHW3wXNjyyA@mail.gmail.com>
 <CAAD30UKTp87+kvGZYL3M2Suxq=WEvFUG24ZRT0yT9rqdkP=uMA@mail.gmail.com>
 <MEYP282MB1669863D5C31D7F6A1D996D8B6139@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
 <CAAD30UKc=GiGQzE8H7+Ofo18hwMOfK4qUm_KUyw6c09q4JvA5Q@mail.gmail.com>
 <MEYP282MB16691E383140844437FB0633B6139@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
 <CAAD30U+ZTBXLH0wWsW9+Zu2RECGKeaQNynLs7wKA0o86w8C-fw@mail.gmail.com>
 <MEYP282MB166926E46397CBFC113B4A7EB6189@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
In-Reply-To: 
 <MEYP282MB166926E46397CBFC113B4A7EB6189@MEYP282MB1669.AUSP282.PROD.OUTLOOK.COM>
From: Amit Kapila <amit.kapila16@gmail.com>
Date: Thu, 7 Apr 2022 15:46:32 +0530
Message-ID: 
 <CAA4eK1J4AekmEKgmfp6e-zZz4M02m7w7uxvC2tjqmjF-LDSGDA@mail.gmail.com>
Subject: Re: Support logical replication of DDLs
To: Japin Li <japinli@hotmail.com>
Cc: Zheng Li <zhengli10@gmail.com>, Alvaro Herrera <alvherre@alvh.no-ip.org>,
	Dilip Kumar <dilipbalaut@gmail.com>, rajesh.rs0541@gmail.com,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAA4eK1J4AekmEKgmfp6e-zZz4M02m7w7uxvC2tjqmjF-LDSGDA%40mail.gmail.com>
Precedence: bulk

On Wed, Mar 23, 2022 at 10:39 AM Japin Li <japinli@hotmail.com> wrote:
>
> Thanks for fixing this.  I rebase the patchset on current master (383f222119)
> and attach here for further review.
>

Some initial comments:
===================
1.
+/*
+ * Write logical decoding DDL message into XLog.
+ */
+XLogRecPtr
+LogLogicalDDLMessage(const char *prefix, Oid roleoid, const char *message,
+ size_t size, bool transactional)

I don't see anywhere the patch using a non-transactional message. Is
it required for any future usage? The non-transactional behavior has
some known consistency issues, see email [1].

2. For DDL replication, do we need to wait for a consistent point of
snapshot? For DMLs, that point is a convenient point to initialize
replication from, which is why we export a snapshot at that point,
which is used to read normal data. Do we have any similar needs for
DDL replication?

3. The patch seems to be creating an entry in pg_subscription_rel for
'create' message. Do we need some handling on Drop, if not, why? I
think adding some comments for this aspect would make it easier to
follow.

4. The handling related to partition tables seems missing because, on
the subscriber-side, it always creates a relation entry in
pg_subscription_rel which won't work. Check its interaction with
publish_via_partition_root.

[1] - https://www.postgresql.org/message-id/CAA4eK1KAFdQEULk%2B4C%3DieWA5UPSUtf_gtqKsFj9J9f2c%3D8hm4g%40mail.gmail.com

-- 
With Regards,
Amit Kapila.