Re: row filtering for logical replication

On Tue, Jan 4, 2022 at 9:58 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Here is the v58* patch set:
>
> Main changes from v57* are
> 1. Couple of review comments fixed
>
> ~~
>
> Review comments (details)
> =========================
>
> v58-0001 (main)
> - PG docs updated as suggested [Alvaro, Euler 26/12]
>
> v58-0002 (new/old tuple)
> - pgputput_row_filter_init refactored as suggested [Wangw 30/12] #3
> - re-ran pgindent
>
> v58-0003 (tab, dump)
> - no change
>
> v58-0004 (refactor transformations)
> - minor changes to commit message

Few comments:
1) We could include namespace names along with the relation to make it
more clear to the user if the user had specified tables having same
table names from different schemas:
+ /* Disallow duplicate tables if there are any
with row filters. */
+ if (t->whereClause ||
list_member_oid(relids_with_rf, myrelid))
+ ereport(ERROR,
+
(errcode(ERRCODE_DUPLICATE_OBJECT),
+ errmsg("conflicting
or redundant WHERE clauses for table \"%s\"",
+
RelationGetRelationName(rel))));

2) Few includes are not required, I could compile without it:
#include "executor/executor.h" in pgoutput.c,
#include "parser/parse_clause.h",
#include "parser/parse_relation.h" and #include "utils/ruleutils.h" in
relcache.c and
#include "parser/parse_node.h" in pg_publication.h

3) I felt the 0004-Row-Filter-refactor-transformations can be merged
to 0001 patch, since most of the changes are from 0001 patch or the
functions which are moved from pg_publication.c to publicationcmds.c
can be handled in 0001 patch.

4) Should this be posted as a separate patch in a new thread, as it is
not part of row filtering:
--- a/src/include/parser/parse_node.h
+++ b/src/include/parser/parse_node.h
@@ -79,7 +79,7 @@ typedef enum ParseExprKind
EXPR_KIND_CALL_ARGUMENT, /* procedure argument in CALL */
EXPR_KIND_COPY_WHERE, /* WHERE condition in COPY FROM */
EXPR_KIND_GENERATED_COLUMN, /* generation expression for a column */
- EXPR_KIND_CYCLE_MARK, /* cycle mark value */
+ EXPR_KIND_CYCLE_MARK /* cycle mark value */
} ParseExprKind;

5) This log will be logged for each tuple, if there are millions of
records it will get logged millions of times, we could remove it:
+ /* update requires a new tuple */
+ Assert(newtuple);
+
+ elog(DEBUG3, "table \"%s.%s\" has row filter",
+
get_namespace_name(get_rel_namespace(RelationGetRelid(relation))),
+ get_rel_name(relation->rd_id));

Regards,
Vignesh