Re: row filtering for logical replication

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.

2. I think your previous version code in GetRelationPublicationActions
was better as now we have to call memcpy at two places.

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.

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?

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.

--
With Regards,
Amit Kapila.