Re: [HACKERS] Runtime Partition Pruning
От | Amit Langote |
---|---|
Тема | Re: [HACKERS] Runtime Partition Pruning |
Дата | |
Msg-id | ae3375a4-be68-b101-36c3-1fa7af5dea10@lab.ntt.co.jp обсуждение исходный текст |
Ответ на | Re: [HACKERS] Runtime Partition Pruning (Amit Langote <Langote_Amit_f8@lab.ntt.co.jp>) |
Ответы |
Re: [HACKERS] Runtime Partition Pruning
(David Rowley <david.rowley@2ndquadrant.com>)
Re: [HACKERS] Runtime Partition Pruning (David Rowley <david.rowley@2ndquadrant.com>) |
Список | pgsql-hackers |
On 2018/04/05 12:14, Amit Langote wrote: > I will post comments on your v19 later today. I looked at it and here are my thoughts on it after having for the first time looked very closely at it. * Regarding PartitionPruneInfo: I think the names of the fields could be improved -- like pruning_steps instead prunesteps, unpruned_parts instead of allpartindexs. The latter is even a bit misleading because it doesn't in fact contain *all* partition indexes, only those that are unpruned, because each either has a subpath or it appears in (unpruned) partitioned_rels list. Also, I didn't like the name subnodeindex and subpartindex very much. subplan_indexes and parent_indexes would sound more informative to me. * make_partition_pruneinfo has a parameter resultRelations that's not used anywhere * In make_partition_pruneinfo() allsubnodeindex = palloc(sizeof(int) * root->simple_rel_array_size); allsubpartindex = palloc(sizeof(int) * root->simple_rel_array_size); I think these arrays need to have root->simple_rel_array_size + 1 elements, because they're subscripted using RT indexes which start at 1. * Also in make_partition_pruneinfo() /* Initialize to -1 to indicate the rel was not found */ for (i = 0; i < root->simple_rel_array_size; i++) { allsubnodeindex[i] = -1; allsubpartindex[i] = -1; } Maybe, allocate the arrays above mentioned using palloc0 and don't do this initialization. Instead make the indexes that are stored in these start with 1 and consider 0 as invalid entries. * Regarding the code added in execPartition.c and execPartition.h: I wondered why PartitionedRelPruning is named the way it is. I saw many parallels with PartitionDispatchData both in terms of the main thing it consists of, that is, the map that translates partition indexes as in partition descriptor to that of subplans or of some other executor structure. Also, I imagine you tried to mimic PartitionTupleRouting with PartitionPruning but not throughout. For example, tuple routing struct pointer variables are throughout called proute, whereas PartitionPruning ones are called partprune instead of, say, pprune. Consistency would help, imho. * Instead of nesting PartitionedRelPruning inside another, just store them in a global flat array in the PartitionPruning, like PartitionTupleRouting does for PartitionDispatch of individual partitioned tables in the tree. typedef struct PartitionedRelPruning { int nparts; int *subnodeindex; struct PartitionedRelPruning **subpartprune; * I don't see why there needs to be nparts in the above, because it already has a PartitionPruneContext member which has that information. In fact, I made most of changes myself while going through the code. Please see attached the delta patch. It also tweaks quite a few other things including various comments. I think parts of it apply to 0001, 0003, and 0004 patches. See if this looks good to you. Thanks, Amit
Вложения
В списке pgsql-hackers по дате отправления:
Следующее
От: David RowleyДата:
Сообщение: Re: Get the name of the target Relation from Query struct?