On Tues, Jan 18, 2022 9:27 AM Peter Smith <smithpb2...@gmail.com> wrote: > Here are some review comments for v66-0001 (review of updates since > v65-0001)
Thanks for the comments! > ~~~ > > 1. src/backend/catalog/pg_publication.c - GetTopMostAncestorInPublication > > @@ -276,17 +276,45 @@ GetPubPartitionOptionRelations(List *result, > PublicationPartOpt pub_partopt, } > > /* > + * Returns the relid of the topmost ancestor that is published via this > + * publication if any, otherwise return InvalidOid. > + */ > > Suggestion: > "otherwise return InvalidOid." --> "otherwise returns InvalidOid." > Changed. > > 2. src/backend/commands/publicationcmds.c - > contain_invalid_rfcolumn_walker > > @@ -235,6 +254,337 @@ CheckObjSchemaNotAlreadyInPublication(List > *rels, List *schemaidlist, > } > > /* > + * Returns true, if any of the columns used in the row filter WHERE > + clause are > + * not part of REPLICA IDENTITY, false, otherwise. > > Suggestion: > ", false, otherwise" --> ", otherwise returns false." > Changed. > > 3. src/backend/replication/logical/tablesync.c - fetch_remote_table_info > > + * We do need to copy the row even if it matches one of the > + publications, > + * so, we later combine all the quals with OR. > > Suggestion: > > BEFORE > * We do need to copy the row even if it matches one of the publications, > * so, we later combine all the quals with OR. > AFTER > * We need to copy the row even if it matches just one of the publications, > * so, we later combine all the quals with OR. > Changed. > > 4. src/backend/replication/pgoutput/pgoutput.c - > pgoutput_row_filter_exec_expr > > + ret = ExecEvalExprSwitchContext(state, econtext, &isnull); > + > + elog(DEBUG3, "row filter evaluates to %s (isnull: %s)", > + DatumGetBool(ret) ? "true" : "false", > + isnull ? "false" : "true"); > + > + if (isnull) > + return false; > + > + return DatumGetBool(ret); > > That change to the logging looks incorrect - the "(isnull: %s)" value is > backwards now. > > I guess maybe the intent was to change it something like below: > > elog(DEBUG3, "row filter evaluates to %s (isnull: %s)", isnull ? "false" : > DatumGetBool(ret) ? "true" : "false", isnull ? "true" : "false"); I misread the previous comments. I think the original log is correct and I have reverted this change. Best regards, Hou zj
