Re: row filtering for logical replication

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.

> I think we can now merge 0001, 0002, and 0005. We are still evaluating
> the performance for 0003, so it is better to keep it separate. We can
> take the decision to merge it once we are done with our evaluation.
>

Merged as suggested in v51* [1]

------
[1] https://www.postgresql.org/message-id/CAHut%2BPs%2BdACvefCZasRE%3DP%3DDtaNmQvM3kiGyKyBHANA0yGcTZw%40mail.gmail.com

Kind Regards,
Peter Smith.
Fujitsu Australia