On Thu, Jan 20, 2022 at 7:51 AM Peter Smith <smithpb2...@gmail.com> wrote:
>
> Here are some review comments for v68-0001.
>
>
> 3. src/backend/replication/pgoutput/pgoutput.c - pgoutput_row_filter_init
>
> + /* If no filter found, clean up the memory and return */
> + if (!has_filter)
> + {
> + if (entry->cache_expr_cxt != NULL)
> + MemoryContextDelete(entry->cache_expr_cxt);
> +
> + entry->exprstate_valid = true;
> + return;
> + }
>
> IMO this should be refactored to have if/else, so the function has
> just a single point of return and a single point where the
> exprstate_valid is set. e.g.
>
> if (!has_filter)
> {
> /* If no filter found, clean up the memory and return */
> ...
> }
> else
> {
> /* Create or reset the memory context for row filters */
> ...
> /*
> * Now all the filters for all pubactions are known. Combine them when
> * their pubactions are same.
> ...
> }
>
> entry->exprstate_valid = true;
>
This part of the code is changed in v68 which makes the current code
more suitable as we don't need to deal with memory context in if part.
I am not sure if it is good to add the else block here but I think
that is just a matter of personal preference.
--
With Regards,
Amit Kapila.
