Re: PG DOCS - logical replication filtering

On Fri, Mar 11, 2022 at 9:37 AM Euler Taveira <euler(at)eulerto(dot)com> wrote:
>
> On Fri, Mar 4, 2022, at 12:41 AM, Peter Smith wrote:
>
> PSA patch v3 to address all comments received so far.
>
> Peter,
>
> I started reviewing this documentation for row filter and I noticed that I was
> changing too much lines to submit a patch on the top of it. Hence, I'm
> attaching a new version based on this one.

Sorry for my long delay in replying. I have been away.

Thanks very much for the updated patch with all your suggestions.
There were many changes in your v4. I have not merged everything
exactly, but I did take the majority of your suggestions on-board in
the attached v5.

I responded below to each change.

>
> Here as some of the changes that I did:
>
> * links: I renamed the ids to use "logical-replication-row-filter" instead of
> "logical-replication-rf" because it is used in the anchors. A compound word
> is better than an abbreviation.

OK, done as suggested.

> * titles: I changed all titles because of some stylish issue (words are
> generally capitalized) or because it reads better.

OK, most titles changed as suggested.

> * sections: I moved the "Restrictions" section to the top but it seems
> important than the other sections. I removed the "psql" section. The psql
> commands are shown in the "Examples" section so I don't think we should
> provide a section for it.

OK, moved the "Restrictions" section and removed the "psql" section.

> * sentences: I expanded several sentences to provide details about the specific
> topic. I also reordered some sentences and removed some duplicated sentences.

I did not take everything exactly as in your v4, but wherever your
suggested changes added some more information I tried to include them
using similar wording to yours.

> * replica identity: I added a link to replica identity. It is a key concept for
> row filter.

OK, done as suggested.

> * transformations: I added a few sentences explaining when/why a transformation
> is required. I removed the "Cases" part (same as in the source code) because
> it is redundant with the new sentences.

OK, I incorporated most of your new descriptions for the
transformations, however I still wanted to keep the summary of "cases"
part because IMO it makes the rules so much clearer than just having
the text descriptions.

> * partitioned table: the title should be _partitioned_ table. I replaced the
> bullets with sentences in the same paragraph.

OK. The title was changed, but I kept the bullets.

> * initial data sync: I removed the "subscriber" from the section title. When
> you say "initial data synchronization" it seems clear we're talking about the
> subscriber. I also add a sentence saying why pre-15 does not consider row
> filters.

OK. Title is changed. Also I added your sentence about the pre-15. But
I kept all the HTML note rendering that you'd removed in v4. I think
this information was important enough to be a "note" instead of just
buried in a paragraph.

> * combining row filters: I renamed the section and decided to remove "for the
> same table". When reading the first sentences from this section, it is clear
> that row filtering is per table. So if you are combining multiple row
> filters, it should be for the same table. I also added a sentence saying why
> some clauses make the row filter irrelevant.

OK. Title is changed. Your extra sentence was added.

> * examples: I combined the psql commands that shows row filter information
> together (\dRp+ and \d). I changed to connection string to avoid "localhost".
> Why? It does not seem a separate service (there is no port) and setup pub/sub
> in the same cluster requires additional steps. It is better to illustrate
> different clusters (at least it seems so since we don't provide details from
> publisher). I changed a value in an UPDATE because both UPDATEs use 999.
>

I did not combine those \dRp+ and \d. I kept them separate so I could
write some separate notes about them.

I'm unsure about the connection change. This documentation is for "Row
Filters" so the examples were only intended to demonstrate row filters
and nothing else. I wanted to have a trivial connection such that a
user can just cut/paste directly from the example and get something
they can immediately test without having to change it. I don't mind
changing this later but probably I'd like to get some other opinions
about it first.

About the UPDATE (555 value) - OK, I changed that value as you suggested.

> We could probably reduce the number of rows in the example but I didn't bother
> to remove them.
>
> It seems we can remove some sentences from the CREATE PUBLICATION because we
> have a new section that explains all of it. I think the link that was added by
> this patch is sufficient.
>

Yeah, maybe some sentences can be removed. But even though some
information is duplicated it might be useful to have a few things
still mentioned on the CREATE PUBLICATION page just so the user does
not have to chase links too much. I will wait to see if other people
have an opinion about it before removing any content from that page.
Meanwhile, I have made the create_publication.sgml identical to your
v4.

~~~

There should be much fewer v4/v5 differences now although there might
be a few things I missed that you want to re-comment on. Hopefully, it
will now be easier to post review comments as BEFORE/AFTER text
fragments - that would help other people to see the suggestions more
easily and give their opinions too.

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