Re: [sqlsmith] Failed assertion during partition pruning

On Mon, Feb 1, 2021 at 8:50 AM David Rowley <dgrowleyml(at)gmail(dot)com> wrote:
> On Sun, 31 Jan 2021 at 11:42, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> > This fixes the cases reported by Andreas and Jaime, leaving me
> > more confident that there's nothing wrong with David's Assert.
>
> It could be fixed by modifying get_singleton_append_subpath() to
> modify the partitioned_rels when it collapses these single-subpath
> Append/MergeAppend path, but I'm very open to looking at just getting
> rid of the partitioned_rels field. Prior to a929e17e5 that field was
> very loosely set and in serval cases could contain RT indexes of
> partitioned tables that didn't even have any subpaths in the given
> Append/MergeAppend. a929e17e5 attempted to tighten all that up but
> looks like I missed the case above.
>
> > I wonder whether we should consider back-patching this. Another
> > thing that seems unclear is whether there is any serious consequence
> > to omitting some intermediate partitions from the set considered
> > by make_partition_pruneinfo. Presumably it could lead to missing
> > some potential run-time-pruning opportunities, but is there any
> > worse issue? If there isn't, this is a bigger change than I want
> > to put in the back braches.
>
> It shouldn't be backpatched. a929e17e5 only exists in master. Prior to
> that AppendPath/MergeAppendPath's partitioned_rels field could only
> contain additional partitioned table RT index. There are no known
> cases of missing ones prior to a929e17e5, so this bug shouldn't exist
> in PG13.
>
> a929e17e5 introduced run-time partition pruning for
> sub-Append/MergeAppend paths. The commit message of that explains
> that there are plenty of legitimate cases where we can't flatten these
> sub-Append/MergeAppend paths down into the top-level
> Append/MergeAppend. Originally I had thought we should only bother
> doing run-time pruning on the top-level Append/MergeAppend because I
> thought these cases were very uncommon. I've now changed my mind.
>
> For a929e17e5, it was not just a matter of removing the lines in [1]
> to allow run-time pruning on nested Append/MergeAppends. I also needed
> to clean up the sloppy setting of partitioned_rels. The remainder of
> the patch attempted that.
>
> FWIW, I hacked together a patch which fixes the bug by passing a
> Bitmapset ** pointer to get_singleton_append_subpath(), which set the
> bits for the Append / MergeAppend path's partitioned_rels that we get
> rid of when it only has a single subpath. This stops the Assert
> failure mentioned here. However, I'd much rather explore getting rid
> of partitioned_rels completely. I'll now have a look at the patch
> you're proposing for that.

I've read Tom's patch (0001) and would definitely vote for that over
having partitioned_rels in Paths anymore.

> Thanks for investigating this and writing the patch.

+1. My apologies as well for failing to notice this thread sooner.

--
Amit Langote
EDB: http://www.enterprisedb.com