Re: row filtering for logical replication

On Sat, Jan 15, 2022 at 12:00 PM houzj(dot)fnst(at)fujitsu(dot)com
<houzj(dot)fnst(at)fujitsu(dot)com> wrote:
>
> On Friday, January 14, 2022 7:31 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
> > On Thu, Jan 13, 2022 at 6:46 PM houzj(dot)fnst(at)fujitsu(dot)com
> >
> > 9.
> > --- a/src/backend/replication/logical/proto.c
> > +++ b/src/backend/replication/logical/proto.c
> > @@ -15,6 +15,7 @@
> > #include "access/sysattr.h"
> > #include "catalog/pg_namespace.h"
> > #include "catalog/pg_type.h"
> > +#include "executor/executor.h"
> >
> > Do we really need this include now? Please check includes in other
> > files as well and remove if anything is not required.
> >
...
....
>
> Thanks for the comments.
> Attach the V65 patch set which addressed the above comments and Peter's comments[1].

The above comment (#9) doesn't seem to be addressed. Also, please
check other includes as well. I find below include also unnecessary.

--- a/src/backend/replication/pgoutput/pgoutput.c
+++ b/src/backend/replication/pgoutput/pgoutput.c
...
...
+#include "nodes/nodeFuncs.h"

Some other comments:
==================
1.
/*
+ * If we know everything is replicated and some columns are not part
+ * of replica identity, there is no point to check for other
+ * publications.
+ */
+ if (pubactions.pubinsert && pubactions.pubupdate &&
+ pubactions.pubdelete && pubactions.pubtruncate &&
+ (!validate_rowfilter || !rfcol_valid))
break;

Why do we need to continue for other publications after we find there
is an invalid column in row_filter?

2.
* For initial synchronization, row filtering can be ignored in 2 cases:
+ *
+ * 1) one of the subscribed publications has puballtables set to true
+ *
+ * 2) one of the subscribed publications is declared as ALL TABLES IN
+ * SCHEMA that includes this relation

Isn't there one more case (when one of the publications has a table
without any filter) where row filtering be ignored? I see that point
being mentioned later but it makes things unclear. I have tried to
make things clear in the attached.

Apart from the above, I have made a few other cosmetic changes atop
v65-0001*.patch. Kindly review and merge into the main patch if you
are okay with these changes.

--
With Regards,
Amit Kapila.