Re: Support logical replication of DDLs - Mailing list pgsql-hackers

> Hi, authors on this thread.
>
> The patch v32-0001 is very large, so it will take some time to review
> the code in detail.

Thanks for reviewing!

> Meanwhile, here are some general comments about the patch:
>
> ======
>
> 1. It might be useful to add this thread to the commitfest, if only so
> the cfbot can discover the latest patch set and alert about any rebase
> problems.

There is already a commitfest entry for the thread that I added back in March:
https://commitfest.postgresql.org/40/3595/

> 2. User interface design/documentation?
>
> Please consider adding user interface documentation, so it is
> available for review sooner than later. This comment was previously
> posted 2 weeks ago [1] but no replies.
>
> I can only guess (from the test of patch 0004) that the idea is to use
> another 'ddl' option for the 'publish' parameter:
> CREATE PUBLICATION mypub FOR ALL TABLES with (publish = 'insert,
> update, delete, ddl');
>
> My first impression is that a blanket option like that could be
> painful for some users who DO (for example) want the convenience of
> the DDL replication automagically creating new tables on the fly, but
> maybe they do NOT want the side-effect of replicating every other kind
> of DDL as well.
>
> Maybe such scenarios can be handled by another publication parameter
> which can allow more fine-grained DDL replication like:
> CREATE PUBLICATION mypub FOR ALL TABLES WITH (ddl = 'tables')
>
> I also have lots of usability questions but probably the documentation
> would give the answers to those. IMO the docs for the user interface
> and runtime behaviours should not be deferred - they should form part
> of this patch 0001.

We've been deferring the discussion on user interface syntax (and
documentation) until we
get the DDL deparser in a good shape. I agree it's time
to pick up the discussion again now that we're getting close to fully
integrating
the DDL deparser with DDL replication. I think it makes sense to introduce
different DDL replication granularity levels, for example, I think the
most important levels
would be ddl = 'tables' and ddl = 'database' (or ddl = 'all').

> 5. File size
>
> As mentioned above in #4, the src/backend/commands/ddl_deparse.c is
> huge (9200+ lines as at v32-0001). It is already unwieldy. Is there
> some way to reduce this? For example, perhaps many of those
> "utility/helper" functions (even though they are static) would be
> better moved out to another file simply to get things down to a more
> manageable size.

Yes, I think we can split patch 0001 into a bare-bone patch for a few
essential commands and a patch
for the rest of the commands for ease of review.

Another topic we haven't discussed is the ownership of the replicated
objects. Currently all the replicated
objects are owned by the subscription owner regardless of their owners
in the publisher database. I think
we can consider making it user configurable so that the ownership of
the replicated objects match that of their original owner in
certain use cases such as in a full database logical replica scenario.
Otherwise the DBA will have to
fix the ownership structure manually which could be painful.

Thoughts?

Regards,
Zheng