Re: Support logical replication of DDLs

Hi Amit,

> 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].

The transactional flag is not required by the current usage. I thought
it might be useful if other logical decoding plugins want to log and
consume DDL messages in a non-transactional way. But I don't have a
specific use case yet.

> 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?

The current design requires manual schema initialization on the
subscriber before the logical replication setup.
As Euler Taveira pointed out, snapshot is needed in initial schema
synchronization. And that is a different topic.


> 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.

It's already handled by existing logic in heap_drop_with_catalog:
https://github.com/zli236/postgres/blob/ddl_replication/src/backend/catalog/heap.c#L2005
I'll add some comment.

> 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.

I will test it out.

>Even if this works, how will we make Alter Table statement work where
>it needs to rewrite the table? There also I think we can face a
>similar problem if we directly send the statement, once the table will
>be updated due to the DDL statement and then again due to table
>rewrite as that will have a separate WAL.

Yes, I think any DDL that can generate DML changes should be listed
out and handled properly or documented. Here is one extreme example
involving volatile functions:
ALTER TABLE nd_ddl ADD COLUMN t timestamp DEFAULT now().
Again, I think we need to somehow skip the data rewrite on the
subscriber when replicating such DDL and let DML replication handle
the rewrite.

>Another somewhat unrelated problem I see with this work is how to save
>recursion of the same command between nodes (when the involved nodes
>replicate DDLs). For DMLs, we can avoid that via replication origins
>as is being done in the patch proposed [1] but not sure how will we
>deal with that here?

I'll need to investigate "recursion of the same command between
nodes", could you provide an example?

Regards,
Zheng