On Tue, Jan 4, 2022 at 9:58 AM Peter Smith <smithpb2...@gmail.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