Zhihong Yu <z...@yugabyte.com> writes:
> In remove_unneeded_nulling_relids():
> + if (removable_relids == NULL)
> Why is bms_is_empty() not used in the above check ?
We initialized that to NULL just a few lines above, and then did
nothing to it other than perhaps bms_add_member, so it's impossible
for it to be empty-and-yet-not-NULL.
> +typedef struct reduce_outer_joins_partial_state
> Since there are already reduce_outer_joins_pass1_state
> and reduce_outer_joins_pass2_state, a comment
> above reduce_outer_joins_partial_state would help other people follow its
> purpose.
We generally document these sorts of structs in the using code,
not at the struct declaration.
> + if (j->rtindex)
> + {
> + if (j->jointype == JOIN_INNER)
> + {
> + if (include_inner_joins)
> + result = bms_add_member(result, j->rtindex);
> + }
> + else
> + {
> + if (include_outer_joins)
> Since there are other join types beside JOIN_INNER, should there be an
> assertion in the else block?
Like what? We don't particularly care what the join type is here,
as long as it's not INNER. In any case there is plenty of nearby
code checking for unsupported join types.
regards, tom lane
