RE: row filtering for logical replication

On Wednesday, January 5, 2022 2:45 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Wed, Jan 5, 2022 at 11:04 AM Peter Smith <smithpb2250(at)gmail(dot)com>
> wrote:
> >
> >
> > 11. src/backend/utils/cache/relcache.c - GetRelationPublicationActions
> >
> > Something seemed slightly fishy with the code doing the memcpy,
> > because IIUC is possible for the GetRelationPublicationInfo function
> > to return without setting the relation->rd_pubactions. Is it just
> > missing an Assert or maybe a comment to say such a scenario is not
> > possible in this case because the is_publishable_relation was already
> > tested?
> >
>
> I think it would be good to have an Assert for a valid value of
> relation->rd_pubactions before doing memcpy. Alternatively, in
> function, GetRelationPublicationInfo(), we can have an Assert when
> rd_rfcol_valid is true. I think we can add comments atop
> GetRelationPublicationInfo about pubactions.
>
> >
> > 13. src/include/utils/rel.h - comment typos
> >
> > @@ -164,6 +164,13 @@ typedef struct RelationData
> > PublicationActions *rd_pubactions; /* publication actions */
> >
> > /*
> > + * true if the columns referenced in row filters from all the
> > + publications
> > + * the relation is in are part of replica identity, or the
> > + publication
> > + * actions do not include UPDATE and DELETE.
> > + */
> >
> > Some minor rewording of the comment:
> >
> ...
> > "UPDATE and DELETE" --> "UPDATE or DELETE"
> >
>
> The existing comment seems correct to me. Hou-San can confirm it once as I
> think this is written by him.

I think the code comment is trying to say
"the publication does not include UPDATE and also does not include DELETE"
I am not too sure about the grammar, I noticed there is some other places in
the code use " no updates or deletes ", so maybe it's fine to change it to
"UPDATE or DELETE"

Best regards,
Hou zj