Here are some review comments for v66-0001 (review of updates since v65-0001)
~~~ 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." ~~~ 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." ~~~ 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. ~~~ 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"); ------ Kind Regards, Peter Smith. Fujitsu Australia
