[prev in list] [next in list] [prev in thread] [next in thread] 

List:       pgsql-patches
Subject:    Re: [PATCHES] Ordered Append WIP patch v1
From:       Heikki Linnakangas <heikki () enterprisedb ! com>
Date:       2008-03-31 20:14:53
Message-ID: 47F1463D.7000005 () enterprisedb ! com
[Download RAW message or body]

Gregory Stark wrote:
> Here's the WIP patch I described on -hackers to implemented "ordered" append
> nodes.

I think it would be better to create completely separate executor node 
for this, rather than tack this feature on the regular Append node. The 
two don't seem to have much in common. Looking at AppendState, for 
example, as_firstplan and as_lastplan are not interesting for the 
MergeAppend, and likewise MergeAppend needs a whole bunch of fields not 
interesting to regular Append. The fact that the first statement in 
ExecAppend is "if(!node->as_is_ordered)", with zero lines of shared 
codes between them, also suggests that it would be a good idea to keep 
them separate.

As you point out in the comments, it's quite confusing to have indexes 
into the slots array and the heap array. I still can't get my head 
around that. Would it help to define a struct along the lines of:

struct {
   TupleTableSlot *slot;
   PlanState *plan;
};

and store pointers to those in the heap?

> 1) I still haven't completely figured out what to do with equivalence classes.
>    My hack of just stuffing all the append subrel vars into there seems to
>    work fine. I need to understand what's going on to see if there's really a
>    problem with it or not.

I don't understand that well enough to comment... I presume the FIXME in 
the patch is related to this?

> 4) I haven't handled mark/restore or random access. I think they could be
>    handled and they'll probably be worth the complexity but I'm not sure.

For Mark/restore, I suppose you would mark/restore all subnodes, and 
store/restore the heap. For reversing direction, you would reverse the 
heap. Doesn't sound too hard, but it's probably better to make it work 
without them at first before adding any bells and whistles...

> 5) Is it considering too many paths? Are there some which maybe aren't worth
>    considering? For example, maybe it only makes sense to take best start_cost
>    paths since if that plan doesn't dominate then the best total_cost plan is
>    likely to be the sequential scans + unordered append + sort.

I don't understand this paragraph. Are you referring to this comment in 
the patch:

> /* If we can't find any plans with the right order just add the
>  * cheapest total plan to both paths, we'll have to sort it
>  * anyways. Perhaps consider not generating a startup path for such
>  * pathkeys since it'll be unlikely to be the cheapest startup
>  * cost. */

Having to sort one or two of the sub-plans might not be to be expensive 
at all. For example, having to sort the empty parent relation of a 
partitioned table is essentially free. It's probably never cheaper to 
use the Merge Append if you need to sort *all* of the children, but if 
you can avoid sorting even one of them, it might very well be worthwhile.

> 6) I haven't looked at setops yet but this is particularly attractive for the
>    UNION (as opposed to UNION ALL) case.

Yes. And DISTINCT.

> 7) I copied/adapted a bunch of bits from tuplesort to maintain the heap and do
>    the scankey comparisons. I could refactor that code back into tuplesort but
>    it would mean twisting around tuplesort quite a bit. Essentially it would
>    mean introducing a new type of tuplesort which would start off in
>    FINAL_MERGE state only it would have to behave differently since we don't
>    want it prefetching lots of records like FINAL_MERGE does, I don't think.

Doesn't seem worthwhile. The heap management part of the patch is quite 
short.

-- 
   Heikki Linnakangas
   EnterpriseDB   http://www.enterprisedb.com

-- 
Sent via pgsql-patches mailing list (pgsql-patches@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-patches
[prev in list] [next in list] [prev in thread] [next in thread]