Re: row filtering for logical replication - Mailing list pgsql-hackers

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