Re: row filtering for logical replication

On Thu, Jan 13, 2022 at 6:46 PM houzj.fnst@fujitsu.com
<houzj.fnst@fujitsu.com> wrote:
>
> Attach the V64 patch set which addressed Alvaro, Amit and Peter's comments.
>

Few more comments:
===================
1.
"SELECT DISTINCT pg_get_expr(pr.prqual, pr.prrelid)"
+ "  FROM pg_publication p"
+ "  LEFT OUTER JOIN pg_publication_rel pr"
+ "       ON (p.oid = pr.prpubid AND pr.prrelid = %u),"
+ "  LATERAL pg_get_publication_tables(p.pubname) GPT"
+ " WHERE GPT.relid = %u"
+ "   AND p.pubname IN ( %s );",

Use all aliases either in CAPS or in lower case. Seeing the nearby
code, it is better to use lower case for aliases.

2.
-
+extern Oid GetTopMostAncestorInPublication(Oid puboid, List *ancestors);

It seems like a spurious line removal. I think you should declare it
immediately after GetPubPartitionOptionRelations() to match the order
of functions as they are in pg_publication.c

3.
+ * It is only safe to execute UPDATE/DELETE when all columns referenced in
+ * the row filters from publications which the relation is in are valid -
+ * i.e. when all referenced columns are part of REPLICA IDENTITY, or the

There is no need for a comma after REPLICA IDENTITY.

4.
+ /*
+ * Find what are the cols that are part of the REPLICA IDENTITY.

Let's change this comment as: "Remember columns that are part of the
REPLICA IDENTITY."

5. The function name rowfilter_column_walker sounds goo generic for
its purpose. Can we rename it contain_invalid_rfcolumn_walker() and
move it to publicationcmds.c? Also, can we try to rearrange the code
in GetRelationPublicationInfo() such that row filter validation
related code is moved to a new function contain_invalid_rfcolumn()
which will internally call contain_invalid_rfcolumn_walker(). This new
functions can also be defined in publicationcmds.c.

6.
+ *
+ * If the cached validation result is true, we assume that the cached
+ * publication actions are also valid.
+ */
+AttrNumber
+GetRelationPublicationInfo(Relation relation, bool validate_rowfilter)

Instead of having the above comment, can we have an Assert for valid
relation->rd_pubactions when we are returning in the function due to
rd_rfcol_valid. Then, you can add a comment (publication actions must
be valid) before Assert.

7. I think we should have a function check_simple_rowfilter_expr()
which internally should call rowfilter_walker. See
check_nested_generated/check_nested_generated_walker. If you agree
with this, we can probably change the name of row_filter function to
check_simple_rowfilter_expr_walker().

8.
+ if (pubobj->pubtable && pubobj->pubtable->whereClause)
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("WHERE clause for schema not allowed"),

Will it be better to write the above message as: "WHERE clause not
allowed for schema"?

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.

10.
 /*
- * Get information about remote relation in similar fashion the RELATION
- * message provides during replication.
+ * Get information about a remote relation, in a similar fashion to how the
+ * RELATION message provides information during replication.

Why this part of the comment needs to be changed?

11.
/*
  * For non-tables, we need to do COPY (SELECT ...), but we can't just
- * do SELECT * because we need to not copy generated columns.
+ * do SELECT * because we need to not copy generated columns.

I think here comment should say: "For non-tables and tables with row
filters, we need to do...."

Apart from the above, I have modified a few comments which you can
find in the attached patch v64-0002-Modify-comments. Kindly, review
those and if you are okay with them then merge those into the main
patch.

-- 
With Regards,
Amit Kapila.