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

Here are some more review comments for the patch 0002-2023_04_07-2

Note: This is a WIP review (part 2); the comments in this post are
only for the commit message and the PG docs

======
Commit message

1.
It's not obvious that those "-" (like "- For DROP object:") represent
major sections of this commit message. For example, at first, I could
not tell that the "We do this way because..." referred to the previous
section; Also it was not clear "TO IMPROVE:" is just an improvement
for the last section, not the entire patch.

In short, I think those main headings should be more prominent so the
commit message is clearly split into these sections.

e.g.
OVERVIEW
--------

For non-rewrite ALTER object command and CREATE object command:
---------------------------------------------------------------

For DROP object:
----------------

For table_rewrite ALTER TABLE command:
-=------------------------------------

~~~

2.
To support DDL replication, it use event trigger and DDL deparsing
facilities. During CREATE PUBLICATION we register a command end trigger that
deparses the DDL (if the DDL is annotated as ddlreplok for DDL replication in
cmdtaglist.h) as a JSON blob, and WAL logs it. The event trigger is
automatically
removed at the time of DROP PUBLICATION. The WALSender decodes the WAL and sends
it downstream similar to other DML commands. The subscriber then converts JSON
back to the DDL command string and executes it. In the subscriber, we also add
the newly added rel to pg_subscription_rel so that the DML changes on the new
table can be replicated without having to manually run
"ALTER SUBSCRIPTION ... REFRESH PUBLICATION".

~

2a.
"it use event trigger" --> "we use the event trigger"

~

2b.
Maybe put 'command start' in quotes as you did later for 'command end'
in this commit message

~~~

3.
- For non-rewrite ALTER object command and
-     CREATE object command:
we deparse the command at ddl_command_end event trigger and WAL log the
deparsed json string. The WALSender decodes the WAL and sends it to
subscriber if the created/altered table is published. It supports most of
ALTER TABLE command except some commands(DDL related to PARTITIONED TABLE
...) that introduced recently which haven't been supported by the current
ddl_deparser, we will support that later.

~

3a.
"we deparse" --> "We deparse"

~

3b.
"that introduced recently which haven't been" --> "that are recently
introduced but are not yet"

~

3c.
Is this information about unsupported ddl_parser stuff still accurate
or is patch 0001 already taking care of this?

~~~

4.
The 'command start' event handler logs a ddl message with the relids of
the tables that are dropped which the output plugin (pgoutput) stores in
its internal data structure after verifying that it is for a table that is
part of the publication. Later the 'command end' event handler sends the
actual drop message. Pgoutput on receiving the command end, only sends out
the drop command only if it is for one of the relids marked for deleting.

~

BEFORE
Pgoutput on receiving the command end, only sends out the drop command
only if it is for one of the relids marked for deleting.

SUGGESTION
On receiving the 'command end', pgoutput sends the DROP command only
if it is for one of the relids marked for deletion.

~~~

5.
The reason we have to do this is because, once the logical decoder
receives the 'command end' message,  the relid of the table is no longer
valid as it has been deleted as part of invalidations received for the
drop table command. It is no longer possible to verify if the table is
part of the publication list or not. To make this possible, I have added
two more elements to the ddl xlog and ddl message, (relid and cmdtype).


~

"I have added two more elements to..." ==> "two more elements are added to..."

~~~

6.
We could have also handled all this on the subscriber side as well, but
that would mean sending spurious ddl messages for tables that are not part
of the publication.

~

"as well" <-- not needed.

~~~

7.
- For table_rewrite ALTER TABLE command:
(ALTER COLUMN TYPE, ADD COLUMN DEFAULT, SET LOGGED, SET ACCESS METHOD)

we deparse the command and WAL log the deparsed json string at
table_rewrite event trigger. The WALSender decodes the WAL and sends it to
subscriber if the altered table is published. Then, the WALSender will
convert the upcoming rewrite INSERTs to UPDATEs and send them to
subscriber so that the data between publisher and subscriber can always be
consistent. Note that the tables that publish rewrite ddl must have a
replica identity configured in order to be able to replicate the upcoming
rewrite UPDATEs.

~

7.
"we deparse" --> "We deparse"

~~~

8.
(1) The data before the rewrite ddl could already be different among
publisher and subscriber. To make sure the extra data in subscriber which
doesn't exist in publisher also get rewritten, we need to let the
subscriber execute the original rewrite ddl to rewrite all the data at
first.

(2) the data after executing rewrite ddl could be different among
publisher and subscriber(due to different functions/operators used during
rewrite), so we need to replicate the rewrite UPDATEs to keep the data
consistent.

~

8a.
"get rewritten" --> "gets rewritten"

~

8b.
"at first." --> "first." ??

~

8c.
First words "The data" versus "the data"

~

8d.
"(due" --> " (due"

~~~

9.
TO IMPROVE:
This approach could be improved by letting the subscriber try to update
the extra data itself instead of doing fully rewrite ddl and use the
upcoming rewrite UPDATEs to rewrite the rest data. To achieve this, we
could modify the deparsed json string to temporarily remove the rewrite
part and add some logic in subscriber to update the extra data.
Besides, we may not need to send rewrite changes for all type of rewrite
ddl, for example, it seems fine to skip sending rewrite changes for ALTER
TABLE SET LOGGED as the data in the table doesn't actually be changed. We
could use the deparser and event trigger to filter these ddls and skip
sending rewrite changes for them.

~

"rest data." --> "rest of the data."

======
doc/src/sgml/logical-replication.sgml

10.
+  <para>
+    Data Definition Commands (DDLs) can be replicated using logical
replication.
+    While enabled this feature automatically replicates supported DDL commands
+    that are successfully executed on a publisher to a subscriber. This is
+    especially useful if you have lots schema changes over time that
need replication.
+  </para>

10a.
"Data Definition Commands (DDLs)" --> "Data Definition Language (DDL) commands"

~

10b.
"While enabled..." --> "When enabled..."

~
10c,
"lots schema" --> "many schema"

~~~

11.
+  <para>
+    For example, when enabled a CREATE TABLE command executed on the
publisher gets
+    WAL-logged, and forwarded to the subscriber to replay; a subsequent "ALTER
+    SUBSCRIPTION ... REFRESH PUBLICATION" is run on the subscriber
database so any
+    following DML changes on the new table can be replicated without a hitch.
+  </para>

~

11a
"For example, when enabled a CREATE TABLE..." --> "For example, a
CREATE TABLE..."

~

11b.
BEFORE
a subsequent "ALTER SUBSCRIPTION ... REFRESH PUBLICATION" is run on
the subscriber...

SUGGESTION
then an implicit "ALTER SUBSCRIPTION ... REFRESH PUBLICATION" is
performed on the subscriber...

~

11c.
"without a hitch" <-- That seemed a bit too colloquial IMO.

~~~

12.
+  <para>
+    DDL replication is disabled by default, it can be enabled at
different levels
+    using the ddl PUBLICATION option. This option currently has one
level and are
+    only allowed to be set if the PUBLICATION is FOR ALL TABLES or
FOR TABLES IN SCHEMA.
+  </para>


12a.
it can be enabled at different levels <-- No it can't yet.

This option currently has one level <--  IMO don't say it that way.
Instead, just say the one level ddl='table' that is CAN do now. In the
future when more can be done then those will be added to the docs.

~

12b.
There should be more documentation for the ddl parameter on the CREATE
PUBLICATION docs page and this should link to it.

~

12c.
There should also be cross-refs to the "FOR ALL TABLES" and "FOR ALL
TABLES IN SCHEMA" xrefs. See other LR SGML documentation for how we
did all this recently.

~~~

13.
+  <sect2 id="ddl-replication-option-examples">
+    <title>Examples - Setup DDL Replication on the Publisher</title>
+
+    <para>
+      Enable table ddl replication for an existing Publication:
+<programlisting>
+ALTER PUBLICATION mypub SET (ddl = 'table');
+</programlisting></para>
+
+  </sect2>

13a.
"table ddl replication" --> "TABLE DDL replication"

~

13b
"Publication" --> "PUBLICATION"

~

13c.
IMO should also be an example using CREATE PUBLICATION

~~~

14.
+    <para>
+      The DDL commands supported for logical replication are listed
in the following
+      matrix. Note that global commands can be executed at any
database and are currently
+      not supported for replication, global commands include ROLE
statements, Database
+      statements, TableSpace statements and some of the
GrantStmt/RevokeStmt if the target
+      object is a global object. Temporary and unlogged objects will
not be replicated.
+      User should take care of creating these objects as these
objects might be required
+      by the objects that are replicated (for example creation of
tables that might
+      refer to an user created tablespace will fail in the subscriber
if the user
+      created tablespaces are not created in the subscriber).
+    </para>

14a.
"in the following matrix" <-- IMO don't call it as matrix. This should
simply be a link the the table.

~

14b
I felt that the whole "Note that..." might warrant actually being in
some <note> SGML tag, so it renders as a proper note.

~

14c.
"User should take care of creating..." --> "Take care when creating..."

~

14d.
"user created" --> "user-created"

~

14e.
"are not created in the subscriber" --> "do not exist on the subscriber"

~~~

15.
+    <table id="ddl-replication-by-command-tag">
+      <title>DDL Replication Support by Command Tag</title>
+      <tgroup cols="3">
+        <colspec colname="col1" colwidth="2*"/>
+        <colspec colname="col2" colwidth="1*"/>
+        <colspec colname="col3" colwidth="1*"/>
+      <thead>
+       <row>
+        <entry>Command Tag</entry>
+        <entry>For Replication</entry>
+        <entry>Notes</entry>
+       </row>
+      </thead>

15a
IMO this table will be more informative if the 2nd column is renamed
to be "ddl = 'table'", then in future you can just add more columns
when there are different values for that option.

~

15b.
Unless you found some precedent in the PG docs for doing it like this,
IMO it will avoid ambiguity to just write "Yes" for valid values
instead of "X" (e.g. like here
https://www.postgresql.org/about/featurematrix/).

~~~

16.
+    <para>
+      The DDL deparser utility is invoked during the replication of
DDLs. The DDL
+      deparser is capable of converting a DDL command into formatted
JSON blob, with
+      the necessary information to reconstruct the DDL commands at
the destination. The
+      benefit of using the deparser output compared to the original
command string
+      includes:

"The benefit ... includes:" --> "The benefits ... include:"

~~~

17.
+      The DDL deparser exposes two SQL functions:
+      <itemizedlist>

I imagine that these SQL functions should be documented elsewhere as well.

Possibly on this page?
https://www.postgresql.org/docs/current/functions-admin.html#FUNCTIONS-REPLICATION

------
Kind Regards,
Peter Smith.
Fujitsu Australia