Thanks Dmitry for your review.
On Mon, May 7, 2018 at 1:06 AM, Dmitry Dolgov <9erthalion6@gmail.com> wrote:
>
> Thank you. Unfortunately, there are already few more conflicts since last time,
> could you rebase again?
Done.
>
> In the meantime, I'm a bit confused by `merge_null_partitions` and why
> partitions with NULL values should be treated separately? It became even more
> confusing when I looked at where this functions is used:
>
> /* If merge is unsuccessful, bail out without any further processing. */
> if (merged)
> merged = merge_null_partitions(outer_bi, outer_pmap, outer_mmap,
> inner_bi, inner_pmap, inner_mmap,
> jointype, &next_index, &null_index,
> &default_index);
>
> Is this commentary correct?
I agree. It's misleading. Removed.
>
> Also, I don't see anywhere in PostgreSQL itself or in this patch a definition
> of the term "NULL partition", can you add it, just to make things clear?
By NULL partition, I mean a list partition which holds NULL values.
Added that clarification in the prologue of merge_null_partitions().
>
> Another question, I see this pattern a lot here when there is a code like:
>
> if (!outer_has_something && inner_has_something)
> {
> // some logic
> }
> else if (outer_has_something && !inner_has_something)
> {
> // some logic symmetric to what we have above
> }
>
> By symmetric I mean that the code is more or less the same, just "inner"
> variables were changed to "outer" (and required types of joins are opposite).
> Is it feasible to actually use completely the same logic, and just change
> "inner"/"outer" variables instead?
I have added handle_missing_partition() for the same purpose. But I
didn't use it in merge_null_partitions(), which I have done in the
attached patches. Now the number of such instances are just at two
places one in merge_default_partitions() and the other in
handle_missing_partition(). But they are different enough not to
extract into a common function.
--
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company
