Re: row filtering for logical replication

On Fri, Nov 19, 2021 at 5:35 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> 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.
>

Okay, that sounds reasonable.

--
With Regards,
Amit Kapila.