RE: row filtering for logical replication

On Tues, Jan 18, 2022 9:27 AM Peter Smith <smithpb2250(at)gmail(dot)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