Re: PATCH: generate fractional cheapest paths in generate_orderedappend_path

On 12/10/21 00:51, Arne Roland wrote:
> Hi,
> 
> thanks for the reply!
> 
> From: Tomas Vondra <tomas.vondra@enterprisedb.com>
> Sent: Thursday, December 2, 2021 20:58
> Subject: Re: PATCH: generate fractional cheapest paths in 
> generate_orderedappend_path
>  > [...]
>  > Well, I mentioned three open questions in my first message, and I don't
>  > think we've really addressed them yet. We've agreed we don't need to add
>  > the incremental sort here, but that leaves
>  >
>  >
>  > 1) If get_cheapest_fractional_path_for_pathkeys returns NULL, should we
>  > default to cheapest_startup or cheapest_total?
> 
> I think it's reasonable to use cheapest_total like we are doing now. I 
> hardly see any reason to change it.

True, it's a reasonable first step.

Either we generate the same plan as today (with cheapest_total), or 
maybe a better one (if we find a cheaper fractional path with matching 
pathkeys). It's true we could do better, but that's life - it's not like 
we consider every possible path everywhere.

> The incremental sort case you mentioned, seems like the only case that 
> plan might be interesting. If we really want that to happen, we probably 
> should check for that separately, i.e. having startup_fractional. Even 
> though this is a fairly special case as it's mostly relevant for 
> partitionwise joins, I'm still not convinced it's worth the cpu cycles. 
> The point is that in most cases factional and startup_fractional will be 
> the same anyways.

I don't think we can simply check for startup_fractional (although, I'm 
not sure I fully understand what would that be). I think what we should 
really do in this case is walking all the paths, ensuring it's properly 
sorted (with either full or incremental sort), and then pick the 
cheapest fractional path from these sorted paths. But I agree that seems 
pretty expensive.

> And I suspect, even if they aren't, solving that from an application 
> developers point of view, is in most cases not that difficult. One can 
> usually match the pathkey. I personally had a lot of real world issues 
> with missing fractional paths using primary keys. I think it's worth 
> noting that everything will likely match the partition keys anyways, 
> because otherwise there is no chance of doing a merge append.

Yeah, I think you're right.

> If I am not mistaken, in case we end up doing a full sort, the 
> cheapest_total path should be completely sufficient.
> 

Definitely true.

>  > 2) Should get_cheapest_fractional_path_for_pathkeys worry about
>  > require_parallel_safe? I think yes, but we need to update the patch.
> 
> I admit, that such a version of 
> get_cheapest_fractional_path_for_pathkeys could be consistent with other 
> functions. And I was confused about that before. But I am not sure what 
> to use require_parallel_safe for. build_minmax_path doesn't care, they 
> are never parallel safe. And afaict generate_orderedappend_paths cares 
> neither, it considers all plans. For instance when it calls 
> get_cheapest_path_for_pathkeys, it sets require_parallel_safe just to 
> false as well.
> 

True as well. It's looks a bit strange, but you're right neither place 
cares about parallel safety.

>  > I'll take a closer look next week, once I get home from NYC, and I'll
>  > submit an improved version for the January CF.
> 
> Thank you for your work! The current patch, apart from the 
> comments/regression tests, seems pretty reasonable to me.
> 

Attached is a cleaned-up patch, with a simple regression test. I'll mark 
this as RFC and get it committed in a couple days.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company