Re: row filtering for logical replication

On Mon, Nov 15, 2021 at 9:31 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
...
>
> Few comments on the latest set of patches (v39*)
> =======================================
> 0001*
> 1.
> ObjectAddress
> -publication_add_relation(Oid pubid, PublicationRelInfo *targetrel,
> +publication_add_relation(Oid pubid, PublicationRelInfo *pri,
> bool if_not_exists)
> {
> Relation rel;
> HeapTuple tup;
> Datum values[Natts_pg_publication_rel];
> bool nulls[Natts_pg_publication_rel];
> - Oid relid = RelationGetRelid(targetrel->relation);
> + Relation targetrel = pri->relation;
>
> I don't think such a renaming (targetrel-->pri) is warranted for this
> patch. If we really want something like this, we can probably do it in
> a separate patch but I suggest we can do that as a separate patch.
>

The name "targetrel" implies it is a Relation. (and historically, this
arg once was "Relation *targetrel").

Then when the PublicationRelInfo struct was introduced the arg name
was not changed and it became "PublicationRelInfo *targetrel". But at
that time PublicationRelInfo was just a simple wrapper for a Relation
so that was probably ok.

But now this Row-Filter patch has added more new members to
PublicationRelInfo, so IMO the name change is helpful otherwise it
seems misleading to continue calling it like it was still just a
Relation.

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