Re: [sqlsmith] Failed assertion during partition pruning

On Sun, 31 Jan 2021 at 11:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> For simplicity of review I divided the patch into two parts.
> 0001 revises make_partition_pruneinfo() and children to identify
> the relevant parent partitions for themselves, which is not too
> hard to do by chasing up the child-to-parent AppendRelInfo links.
> Not formerly documented, AFAICT, is that we want to collect just
> the parent partitions that are between the Append path's own rel
> (possibly equal to it) and the subpaths' rels.  I'd first tried
> to code this by using the top_parent_relids and part_rels[] links
> in the RelOptInfos, but that turns out not to work.  We might
> ascend to a top parent that's above the Append's rel (if considering
> an appendpath for a sub-partition, which happens in partitionwise
> join).  We could also descend to a child at or below some subpath
> level, since there are also cases where subpaths correspond to
> unflattened non-leaf partitions.  Either of those things result
> in failures.  But once you wrap your head around handling those
> restrictions, it's quite simple.

I had a look at this one and it all makes sense and the logic for
obtaining the lineage of parent partitioned tables seems fine.

What I can't understand is why you changed to a List-of-Lists rather
than a List-of-Relids. This makes the patch both bigger than it needs
to be and the processing quite a bit less efficient.

For example, in make_partition_pruneinfo() when you're walking up to
the top-level target partitioned table you must lcons() each new list
member to ensure the children always come after the parents. These
chains are likely to be short so the horrible overheads of the
memmove() in lcons() won't cost that much, but there's more to it.
The other seemingly needlessly slow part is in the
list_concat_unique_ptr() call. This needs to be done for every subpath
in the Append/Merge append. It would be good to get rid of that.
Given, the list are most likely going to be small, but that's still a
quadratic function, so it seems like a good idea to try to avoid using
it if there is another way to do it.

The memory allocations could also be more efficient for Relids rather
than Lists.  Since we're working up from the child to the parent in
the lineage calculation code in make_partition_pruneinfo(), we'll
always allocate the Bitmapset to the correct size right away, rather
than possibly having to increase the size if the next RT index were
not to fit in the current number of words. Of course, with a
List-of-Lists, not every lcons() would require a new allocation, but
there's an above zero chance that it might.

I've attached a version of your 0001 patch which just maintains using
a List-of-Relids. This shrinks the diff down about 3kb.

Parent RT indexes are guaranteed to be lower than their children RT
indexes, so it's pretty simple to figure out the target RT index by
just looking at the lowest set bit.  Doing it this way also simplifies
things as add_part_rel_list() no longer must insist that the sublists
are in parent-to-child order.

David