Re: row filtering for logical replication

On Wed, Dec 22, 2021 at 5:26 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> On Mon, Dec 20, 2021 at 9:30 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> >
> > On Mon, Dec 20, 2021 at 8:41 AM houzj(dot)fnst(at)fujitsu(dot)com
> > <houzj(dot)fnst(at)fujitsu(dot)com> wrote:
> > >
> > > Thanks for the comments, I agree with all the comments.
> > > Attach the V49 patch set, which addressed all the above comments on the 0002
> > > patch.
> > >
> >
> > Few comments/suugestions:
> > ======================
> > 1.
> > + Oid publish_as_relid = InvalidOid;
> > +
> > + /*
> > + * For a partition, if pubviaroot is true, check if any of the
> > + * ancestors are published. If so, note down the topmost ancestor
> > + * that is published via this publication, the row filter
> > + * expression on which will be used to filter the partition's
> > + * changes. We could have got the topmost ancestor when collecting
> > + * the publication oids, but that will make the code more
> > + * complicated.
> > + */
> > + if (pubform->pubviaroot && relation->rd_rel->relispartition)
> > + {
> > + if (pubform->puballtables)
> > + publish_as_relid = llast_oid(ancestors);
> > + else
> > + publish_as_relid = GetTopMostAncestorInPublication(pubform->oid,
> > + ancestors);
> > + }
> > +
> > + if (publish_as_relid == InvalidOid)
> > + publish_as_relid = relid;
> >
> > I think you can initialize publish_as_relid as relid and then later
> > override it if required. That will save the additional check of
> > publish_as_relid.
> >
>
> Fixed in v51* [1]
>
> > 2. I think your previous version code in GetRelationPublicationActions
> > was better as now we have to call memcpy at two places.
> >
>
> Fixed in v51* [1]
>
> > 3.
> > +
> > + if (list_member_oid(GetRelationPublications(ancestor),
> > + puboid) ||
> > + list_member_oid(GetSchemaPublications(get_rel_namespace(ancestor)),
> > + puboid))
> > + {
> > + topmost_relid = ancestor;
> > + }
> >
> > I think here we don't need to use braces ({}) as there is just a
> > single statement in the condition.
> >
>
> Fixed in v51* [1]
>
> > 4.
> > +#define IDX_PUBACTION_n 3
> > + ExprState *exprstate[IDX_PUBACTION_n]; /* ExprState array for row filter.
> > + One per publication action. */
> > ..
> > ..
> >
> > I think we can have this define outside the structure. I don't like
> > this define name, can we name it NUM_ROWFILTER_TYPES or something like
> > that?
> >
>
> Partly fixed in v51* [1], I've changed the #define name but I did not
> move it. The adjacent comment talks about these ExprState caches and
> explains the reason why the number is 3. So if I move the #define then
> half that comment would have to move with it. I thought it is better
> to keep all the related parts grouped together with the one
> explanatory comment, but if you still want the #define moved please
> confirm and I can do it in a future version.
>

Yeah, I would prefer it to be moved. You can move the part of the
comment suggesting three pubactions can be used for row filtering.

--
With Regards,
Amit Kapila.