On Mon, Nov 15, 2021 at 9:31 PM Amit Kapila <amit.kapila16@gmail.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