RE: row filtering for logical replication

On Tues, Jan 18, 2022 9:27 AM Peter Smith <smithpb2250@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