Some review comments for v62-0001. I was midway through reviewing patch v62-0001 when I saw it had already been pushed [1]. I am posting my review comments here anyway.
====== doc/src/sgml/ddl.sgml 1. + <para> + The <literal>pg_conflict</literal> schema (sometimes referred to as the + <emphasis>conflict schema</emphasis>) contains system-managed conflict + log tables used for logical replication conflict tracking. + These tables are created and maintained by the system and are not intended for + direct user manipulation. Unlike <literal>pg_catalog</literal>, the + <literal>pg_conflict</literal> schema is not implicitly included in the + search path, so objects within it must be referenced explicitly or by + adjusting the search path. + </para> 1a. Why say "sometimes"? ~ 1b. I thought the markup should be <firstterm> instead of <emphasis>. ~ 1c. We just said this is referred to as the "conflict schema" so let's do that! /the <literal>pg_conflict</literal> schema is not/the conflict schema is not/ ====== doc/src/sgml/ref/create_subscription.sgml 2. + <para> + The system automatically creates a structured table named + <literal>pg_conflict_log_<subid></literal> in the + <literal>pg_conflict</literal> schema. This allows for easy + querying and analysis of conflicts. + </para> Don't say "pg_conflict schema". Instead, use the term "conflict schema" with the appropriate <link>. ====== doc/src/sgml/ref/drop_subscription.sgml 3. + <para> + If a conflict log table exists for the subscription (that is, when + <link linkend="sql-createsubscription-params-with-conflict-log-destination"> + <literal>conflict_log_destination</literal></link> is set to <literal>table</literal> + or <literal>all</literal>), <command>DROP SUBSCRIPTION</command> automatically drops + the associated conflict log table. + </para> This seems a bit awkwardly worded: SUGGESTION When <link linkend="sql-createsubscription-params-with-conflict-log-destination"><literal>conflict_log_destination</literal></link> is set to <literal>table</literal> or <literal>all</literal>, <command>DROP SUBSCRIPTION</command> automatically drops the associated conflict log table. ====== GENERAL 4. policy.c: + /* + * Conflict log tables are used internally for logical replication + * conflict logging and should not be modified directly, as it could + * disrupt conflict logging. + */ tablecmds.c: + /* + * Conflict log tables are used internally for logical replication + * conflict logging and should not be modified directly, as it could + * disrupt conflict logging. + */ + /* + * Conflict log tables are used internally for logical replication + * conflict logging and should not be altered directly, as it could + * disrupt conflict logging. Direct ALTER commands are already rejected + /* + * Conflict log tables are used internally for logical replication + * conflict logging and should not be referenced by foreign keys, as it + * could disrupt conflict logging. + */ + /* + * Conflict log tables are used internally for logical replication + * conflict logging and should not be modified directly, as it could + * disrupt conflict logging. + */ + /* + * Conflict log tables are used internally for logical replication + * conflict logging and should not be altered directly, as it could + * disrupt conflict logging. + */ trigger.c: + /* + * Conflict log tables are used internally for logical replication + * conflict logging and should not have triggers, as it could disrupt + * conflict logging. + */ + /* + * Conflict log tables are used internally for logical replication + * conflict logging and should not have triggers, as it could disrupt + * conflict logging. + */ rewriteDefine.c: + /* + * Conflict log tables are used internally for logical replication + * conflict logging and should not have rules, as it could disrupt + * conflict logging. + */ + /* + * Conflict log tables are used internally for logical replication + * conflict logging and should not have rules, as it could disrupt + * conflict logging. + */ ~~~ The wording of the comments above is extremely repetitive. That may not be noteworthy if the comment was just in one place, but it's everywhere. How about like: BEFORE Conflict log tables are used internally for logical replication conflict logging and should not have <xxx>, as it could disrupt conflict logging. SUGGESTION Conflict log tables are internal to logical replication and must not have <xxx>, since it could interfere with their operation. ====== src/include/catalog/pg_subscription.h 5. + /* + * Strategy for logging replication conflicts: 'log' - server log only, + * 'table' - conflict log table only, 'all' - both log and table. + */ + text subconflictlogdest BKI_FORCE_NOT_NULL; IIUC, this comment was deliberately aligned for readability for ~50 versions. But then pg_indent wrapped everything. Consider putting it back how it was and using "/**" style comment so that pg_indent will leave it alone. ====== [1] https://github.com/postgres/postgres/commit/a5918fddf10d297c70f7ec9067e9177e0be6d520 Kind Regards, Peter Smith. Fujitsu Australia
