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

Reply via email to