Re: row filtering for logical replication

On Thu, Jan 13, 2022 at 5:49 PM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
>
> Thanks for posting the merged v63.
>
> Here are my review comments for the v63-0001 changes.
>
...
> ~~~
>
> 4. src/backend/replication/pgoutput/pgoutput.c - pgoutput_row_filter comments
>
> +
> +/*
> + * Change is checked against the row filter, if any.
> + *
> + * If it returns true, the change is replicated, otherwise, it is not.
> + *
> + * FOR INSERT: evaluates the row filter for new tuple.
> + * FOR DELETE: evaluates the row filter for old tuple.
> + * For UPDATE: evaluates the row filter for old and new tuple. If both
> + * evaluations are true, it sends the UPDATE. If both evaluations are false, it
> + * doesn't send the UPDATE. If only one of the tuples matches the row filter
> + * expression, there is a data consistency issue. Fixing this issue requires a
> + * transformation.
> + *
> + * Transformations:
> + * Updates are transformed to inserts and deletes based on the
> + * old tuple and new tuple. The new action is updated in the
> + * action parameter. If not updated, action remains as update.
> + *
> + * Case 1: old-row (no match) new-row (no match) -> (drop change)
> + * Case 2: old-row (no match) new row (match) -> INSERT
> + * Case 3: old-row (match) new-row (no match) -> DELETE
> + * Case 4: old-row (match) new row (match) -> UPDATE
> + *
> + * If the change is to be replicated this function returns true, else false.
> + *
> + * Examples:
>
> The function header comment says the same thing 2x about the return values.
>
> The 1st text "If it returns true, the change is replicated, otherwise,
> it is not." should be replaced by the better wording of the 2nd text
> ("If the change is to be replicated this function returns true, else
> false."). Then, that 2nd text can be removed (from where it is later
> in this same comment).

Hi Hou-san, thanks for all the v64 updates!

I think the above comment was only partly fixed.

The v64-0001 comment still says:
+ * If it returns true, the change is replicated, otherwise, it is not.

I thought the 2nd text is better:
"If the change is to be replicated this function returns true, else false."

But maybe it is best to rearrange the whole thing like:
"Returns true if the change is to be replicated, else false."

------
Kind Regards,
Peter Smith.
Fujitsu Australia