On 2019/03/27 15:48, Amit Langote wrote:
> Hi David,
>
> Sorry if this was discussed before, but why does this patch add any new
> code to partprune.c? AFAICT, there's no functionality changes to the
> pruning code.
>
> Both
>
> +bool
> +partkey_is_bool_constant_for_query(RelOptInfo *partrel, int partkeycol)
>
> and
>
> +static bool
> +matches_boolean_partition_clause(RestrictInfo *rinfo, int partkeycol,
> + RelOptInfo *partrel)
>
> seem like their logic is specialized enough to be confined to pathkeys.c,
> only because it's needed there.
>
>
> Regarding
>
> +bool
> +partitions_are_ordered(PlannerInfo *root, RelOptInfo *partrel)
>
> I think this could simply be:
>
> bool
> partitions_are_ordered(PartitionBoundInfo *boundinfo)
>
> and be defined in partitioning/partbounds.c. If you think any future
> modifications to this will require access to the partition key info in
> PartitionScheme, maybe the following is fine:
>
> bool
> partitions_are_ordered(RelOptInfo *partrel)
Noticed a typo.
+ * multiple subpaths then we can't make guarantees about the
+ * order tuples in those subpaths, so we must leave the
order of tuples?
Again, sorry if this was discussed, but I got curious about why
partitions_are_ordered() thinks it can say true even for an otherwise
ordered list-partitioned table, but containing a null-only partition,
which is *always* scanned last. If a query says ORDER BY partkey NULLS
FIRST, then it's not alright to proceed with assuming partitions are
ordered even if partitions_are_ordered() said so.
Related, why does build_partition_pathkeys() pass what it does for
nulls_first parameter of make_pathkey_from_sortinfo()?
cpathkey = make_pathkey_from_sortinfo(root,
keyCol,
NULL,
partscheme->partopfamily[i],
partscheme->partopcintype[i],
partscheme->partcollation[i],
ScanDirectionIsBackward(scandir),
==> ScanDirectionIsBackward(scandir),
0,
partrel->relids,
false);
I think null values are almost always placed in the last partition, unless
the null-accepting list partition also accepts some other non-null value.
I'm not sure exactly how we can determine the correct value to pass here,
but what's there in the patch now doesn't seem to be it.
Thanks,
Amit
