Re: Conflict detection and logging in logical replication

Peter Smith <smithpb2250@gmail.com>

From: Peter Smith <smithpb2250@gmail.com>
To: "Zhijie Hou (Fujitsu)" <houzj.fnst@fujitsu.com>
Cc: Amit Kapila <amit.kapila16@gmail.com>, shveta malik <shveta.malik@gmail.com>, "Hayato Kuroda (Fujitsu)" <kuroda.hayato@fujitsu.com>, Nisha Moond <nisha.moond412@gmail.com>, pgsql-hackers <pgsql-hackers@postgresql.org>, Dilip Kumar <dilipbalaut@gmail.com>, Jan Wieck <jan@wi3ck.info>, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: 2024-08-21T06:44:42Z
Lists: pgsql-hackers
Here are some review comments for the v19-0001 docs patch.

The content seemed reasonable, but IMO it should be presented quite differently.

~~~~

1. Use sub-sections

I expect this logical replication "Conflicts" section is going to
evolve into something much bigger. Surely, it's not going to be one
humongous page of details, so it will be a section with lots of
subsections like all the other in Chapter 29.

IMO, you should be writing the docs in that kind of structure from the
beginning.

For example, I'm thinking something like below (this is just an
example - surely lots more subsections will be needed for this topic):

29.6 Conflicts
29.6.1. Conflict types
29.6.2. Logging format
29.6.3. Examples

Specifically, this v19-0001 patch information should be put into a
subsection like the 29.6.2 shown above.

~~~

2. Markup

+<screen>
+LOG:  conflict detected on relation "schemaname.tablename":
conflict=<literal>conflict_type</literal>
+DETAIL:  <literal>detailed explaination</literal>.
+<literal>Key</literal> (column_name, ...)=(column_name, ...);
<literal>existing local tuple</literal> (column_name,
...)=(column_name, ...); <literal>remote tuple</literal> (column_name,
...)=(column_name, ...); <literal>replica identity</literal>
(column_name, ...)=(column_name, ...).
+</screen>

IMO this should be using markup more like the SQL syntax references.
- e.g. I suggest <synopsis> instead of <screen>
- e.g. I suggest all the substitution parameters (e.g. detailed
explanation, conflict_type, column_name, ...) in the log should use
<replaceable class="parameter"> and use those markups again later in
these docs instead of <literal>

~

nit - typo /explaination/explanation/

~

nit - The amount of scrolling needed makes this LOG format too hard to
see. Try to wrap it better so it can fit without being so wide.

~~~

3. Restructure the list

+   <itemizedlist>
+    <listitem>

I suggest restructuring all this to use a nested list like:

LOG
- conflict_type
DETAIL
- detailed_explanation
- key
- existing_local_tuple
- remote_tuple
- replica_identity

Doing this means you can remove a great deal of the unnecessary junk
words like "of the first sentence in the DETAIL", and "sentence of the
DETAIL line" etc. The result will be much less text but much simpler
text too.

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



Commits

  1. Rename the conflict types for the origin differ cases.

  2. Doc: explain the log format of logical replication conflicts.

  3. Log the conflicts while applying changes in logical replication.

  4. Avoid duplicate table scans for cross-partition updates during logical replication.