On Wed, Jun 22, 2022 at 2:18 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
>
> On Thu, Jun 16, 2022 at 6:07 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> >
> > Thank you for your review comments. Those reported mistakes are fixed
> > in the attached patch v3.
> >
>
> This patch looks mostly good to me except for a few minor comments
> which are mentioned below. It is not very clear in which branch(es) we
> should commit this patch? As per my understanding, this is a
> pre-existing behavior but we want to document it because (a) It was
> not already documented, and (b) we followed it for row filters in
> PG-15 it seems that should be explained. So, we have the following
> options (a) commit it only for PG-15, (b) commit for PG-15 and
> backpatch the relevant sections, or (c) commit it when branch opens
> for PG-16. What do you or others think?
Even though this is a very old docs omission, AFAIK nobody ever raised
it as a problem before. It only became more important because of the
PG15 row-filters. So I think option (a) is ok.
>
> Few comments:
> ==============
> 1.
> >
> - particular event types. By default, all operation types are replicated.
> - (Row filters have no effect for <command>TRUNCATE</command>. See
> - <xref linkend="logical-replication-row-filter"/>).
> + particular event types. By default, all operation types are replicated.
> + These are DML operation limitations only; they do not affect the initial
> + data synchronization copy.
> >
>
> Using limitations in the above sentence can be misleading. Can we
> change it to something like: "These publication specifications apply
> only for DML operations; they do ... ".
>
OK - modified as suggested.
> 2.
> + operations. The publication <literal>pub3b</literal> has a row filter.
>
> In the Examples section, you have used row filter whereas that section
> is later in the docs. So, it is better if you give reference to that
> section in the above sentence (see Section ...).
>
OK - added xref as suggested.
> 3.
> + <para>
> + This parameter only affects DML operations. In particular, the
> + subscription initial data synchronization does not take
> this parameter
> + into account when copying existing table data.
> + </para>
>
> In the second sentence: "... subscription initial data synchronization
> ..." doesn't sound appropriate. Can we change it to something like:
> "In particular, the initial data synchronization (see Section ..) in
> logical replication does not take this parameter into account when
> copying existing table data."?
>
OK - modified and added xref as suggested.
~~
PSA patch v4 to address all the above review comments.
------
Kind Regards,
Peter Smith.
Fujitsu Australia