Re: row filtering for logical replication

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.

--
With Regards,
Amit Kapila.