Feedback-ID: ia2694551:Fastmail
Date: Wed, 7 Dec 2022 13:20:41 +0100
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: vignesh C <vignesh21@gmail.com>
Cc: Zheng Li <zhengli10@gmail.com>, Ajin Cherian <itsajin@gmail.com>,
	Dilip Kumar <dilipbalaut@gmail.com>,
	"houzj.fnst@fujitsu.com" <houzj.fnst@fujitsu.com>,
	Peter Smith <smithpb2250@gmail.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Masahiko Sawada <sawada.mshk@gmail.com>,
	Japin Li <japinli@hotmail.com>,
	rajesh singarapu <rajesh.rs0541@gmail.com>,
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Subject: Re: Support logical replication of DDLs
Message-ID: <20221207122041.hbfj4hen3ibhdzgn@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CALDaNm0Oi50cocMsd5kdBQyFK1wqXOqRQjrQ6_FcGd7BCCHtUA@mail.gmail.com>
Archived-At: 
 <https://www.postgresql.org/message-id/20221207122041.hbfj4hen3ibhdzgn%40alvherre.pgsql>
Precedence: bulk

I think this patch is split badly.

You have:

0001 an enormous patch including some required infrastructure, plus the
DDL deparsing bits themselves.

0002 another enormous (though not as much) patch, this time for
DDL replication using the above.

0003 a bugfix for 0001, which includes changes in both the
infrastructure and the deparsing bits.

0004 test stuff for 0002.

0005 Another bugfix for 0001

0006 Another bugfix for 0001

As presented, I think it has very little chance of being reviewed
usefully.  A better way to go about this, I think, would be:

0001 - infrastructure bits to support the DDL deparsing parts (all these
new functions in ruleutils.c, sequence.c, etc).  That means, everything
(?) that's currently in your 0001 except ddl_deparse.c and friends.
Clearly there are several independent changes here; maybe it is possible
to break it down even further.  This patch or these patches should also
include the parts of 0003, 0005, 0006 that require changes outside of
ddl_deparse.c.
I expect that this patch should be fairly small.

0002 - ddl_deparse.c and its very close friends.  This should not have
any impact on places such as ruleutils.c, sequence.c, etc.  The parts of
the bugfixes (0001, 0005, 0006) that touch this could should be merged
here as well; there's no reason to have them as separate patches.  Some
test code should be here also, though it probably doesn't need to aim to
be complete.
This one is likely to be very large, but also self-contained.

0003 - ddlmessage.c and friends.  I understand that DDL-messaging is
supporting infrastructure for DDL replication; I think it should be its
own patch.  Probably include its own simple-ish test bits.
Not a very large patch.

0004 - DDL replication proper, including 0004.
Probably not a very large patch either, not sure.


Some review comments, just skimming:
- 0002 adds some functions to event_trigger.c, but that doesn't seem to
be their place.  Maybe some new file in src/backend/replication/logical
would make more sense.

- publication_deparse_ddl_command_end has a long strcmp() list; why?
Maybe change things so that it compares some object type enum instead.

- CreatePublication has a long list of command tags; is that good?
Maybe it'd be better to annotate the list in cmdtaglist.h somehow.

- The change in pg_dump's getPublications needs updated to 16.

- Don't "git add" src/bin/pg_waldump/logicalddlmsgdesc.c, just update
its Makefile and meson.build

- I think psql's \dRp should not have the new column at the end.
Maybe one of:
+ Name | Owner | DDL | All tables | Inserts | Updates | Deletes | Truncates | Via root
+ Name | Owner | All tables | DDL | Inserts | Updates | Deletes | Truncates | Via root
+ Name | Owner | All tables | Inserts | Updates | Deletes | Truncates | DDL | Via root
(I would not add the "s" at the end of that column title, also).


-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"Las cosas son buenas o malas segun las hace nuestra opinión" (Lisias)