| From: | Dilip Kumar <dilipbalaut(at)gmail(dot)com> |
|---|---|
| To: | Peter Smith <smithpb2250(at)gmail(dot)com> |
| Cc: | vignesh C <vignesh21(at)gmail(dot)com>, shveta malik <shveta(dot)malik(at)gmail(dot)com>, Amit Kapila <amit(dot)kapila16(at)gmail(dot)com>, Masahiko Sawada <sawada(dot)mshk(at)gmail(dot)com>, Bharath Rupireddy <bharath(dot)rupireddyforpostgres(at)gmail(dot)com>, PostgreSQL Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org> |
| Subject: | Re: Proposal: Conflict log history table for Logical Replication |
| Date: | 2026-01-19 06:08:31 |
| Message-ID: | CAFiTN-uk_LreMX_NN4H-3pC_aLO4Ykow9m4KhYnxF0e3x2qyUg@mail.gmail.com |
| Views: | Whole Thread | Raw Message | Download mbox | Resend email |
| Thread: | |
| Lists: | pgsql-hackers |
On Mon, Jan 12, 2026 at 10:59 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Some review comments for patch v20-0003 (docs)
>
> ======
> doc/src/sgml/logical-replication.sgml
>
> (29.8. Conflicts)
>
> 1.
> + When the <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>, the system
> + automatically creates a new table with a predefined schema to log conflict
> + details. This table is created in the dedicated
> + <literal>pg_conflict</literal> namespace. The name of the
> conflict log table
> + is pg_conflict_<replaceable>subscription_oid</replaceable>. The
> schema of this
> + table is detailed in
> + <xref linkend="logical-replication-conflict-log-schema"/>.
> + </para>
>
> 1a.
> Instead of saying "When the conflict_log_destination is ...", maybe it
> should say "When the conflict_log_destination parameter is ...".
>
> That should be more consistent with the wording used elsewhere in this patch.
Done in v23-0003
> ~
>
> 1b.
> But, on the CREATE SUBSCRIPTION page, the table name is called:
> <literal>pg_conflict_<subid></literal>
>
> Both places should refer to the name using the same format -- the
> CREATE SUBSCRIPTION way looked good to me.
Done as suggested
> ~~~
>
> 2.
> + The conflicting row data, including the original local tuple and
> + the remote tuple, is stored in <type>JSON</type> columns
> (<literal>local_tuple</literal>
> + and <literal>remote_tuple</literal>) for flexible querying and analysis.
> + </para>
>
> As reported previously [1, comment #5], I didn't see any column called
> 'local_tuple' -- did you mean to refer to the elements of the
> 'local_conflicts' array here?
Fixed and adjusted comments accordingly
> ~~~
>
> 3.
> A previous review comment [1, comment #6 ] suggesting making some
> Chapter 29 supsection/s about "Logging conflicts" was not addressed.
> You disagreed?
I am not sure if we need separate sections for this, so does anyone
else have an opinion on this?
> ======
> doc/src/sgml/ref/create_subscription.sgml
>
> 4.
> A previous review comment [1, comment #13 ] suggesting adding some
> links to refer to Chapter 29 for the logging details was not
> addressed. You disagreed?
I have changed this but it is not included in v23, so will be sent
along with the next version.
--
Regards,
Dilip Kumar
Google
| From | Date | Subject | |
|---|---|---|---|
| Next Message | Michael Paquier | 2026-01-19 06:11:03 | Re: Extended Statistics set/restore/clear functions. |
| Previous Message | Chao Li | 2026-01-19 05:47:47 | Re: More speedups for tuple deformation |