RE: Conflict detection and logging in logical replication

On Wednesday, August 21, 2024 2:45 PM Peter Smith <smithpb2250@gmail.com> wrote:
> 
> 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.

I think that's a good idea. But I preferred to do that in a separate
patch(maybe a third patch after the first and second are RFC), because AFAICS
we would need to adjust some existing docs which falls outside the scope of
the current patch.

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

Agreed. I have changed to use <synopsis> and <replaceable>. But for static
words like "Key" or "replica identity" it doesn't look appropriate to use
<replaceable>, so I kept using <literal> for them.

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

I thought about this, but wrapping the sentence would cause the words
to be displayed in different lines after compiling. I think that's inconsistent
with the real log which display the tuples in one line.

Other comments not mentioned above look good to me.

Attach the V20 patch set which addressed above, Shveta[1][2] and Kuroda-san's[3]
comments.

[1] https://www.postgresql.org/message-id/CAJpy0uDUNigg86KRnk4A0KjwY8-pPaXzZ2eCjnb1ydCH48VzJQ%40mail.gmail.com
[2] https://www.postgresql.org/message-id/CAJpy0uARh2RRDBF6mJ7d807DsNXuCNQmEXZUn__fw4KZv8qEMg%40mail.gmail.com
[3] https://www.postgresql.org/message-id/TYAPR01MB5692C4EDD8B86760496A993AF58E2%40TYAPR01MB5692.jpnprd01.prod.outlook.com

Best Regards,
Hou zj