Обсуждение: Append's first_partial_plan
David Rowley wrote in https://postgr.es/m/CAKJS1f8o2Yd=rOP=Et3A0FWgF+gSAOkFSU6eNhnGzTPV7nN8sQ@mail.gmail.com > I've made another pass over the nodeAppend.c code and I'm unable to > see what might cause this, although I did discover a bug where > first_partial_plan is not set taking into account that some subplans > may have been pruned away during executor init. The only thing I think > this would cause is for parallel workers to not properly help out with > some partial plans if some earlier subplans were pruned. I can see no > reason for this to have caused this particular issue since the > first_partial_plan would be 0 with and without the attached fix. While looking at this patch I became curious as to why do we even have first_partial_plan in the first place; it seems to require some strange contortions in the code. Wouldn't it be simpler to have two lists, one for non-partial and another for partial paths? I went to check the original discussion, and this design was indeed considered [1] -- but the idea was discarded because using the list index would lead to simpler code. However, now that we have pruning it seems to me that using the index isn't simpler anymore. Should we revisit this now? [1] https://www.postgresql.org/message-id/CA%2BTgmoZrjAB0bPzbtKgjP2uAP_6XAyuZenU54QuM7XGE_k2Q1g%40mail.gmail.com -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On 18 April 2018 at 07:52, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > While looking at this patch I became curious as to why do we even have > first_partial_plan in the first place; it seems to require some strange > contortions in the code. Wouldn't it be simpler to have two lists, one > for non-partial and another for partial paths? I went to check the > original discussion, and this design was indeed considered [1] -- but > the idea was discarded because using the list index would lead to > simpler code. However, now that we have pruning it seems to me that > using the index isn't simpler anymore. Should we revisit this now? > > [1] https://www.postgresql.org/message-id/CA%2BTgmoZrjAB0bPzbtKgjP2uAP_6XAyuZenU54QuM7XGE_k2Q1g%40mail.gmail.com I don't think having two Lists and/or two AppendState arrays would make the pruning code anymore simple. All the pruning code in execPartition.c would need to determine the index within the partial or non-partial subnode array, and also communicate which array it means. That code did take me a while to get right and be readable too, I don't really want to have to change it again. I really don't think it would look quite as simple as it does today either, so -1 from me for changing this. -- David Rowley http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
David Rowley wrote: > On 18 April 2018 at 07:52, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote: > > While looking at this patch I became curious as to why do we even have > > first_partial_plan in the first place; it seems to require some strange > > contortions in the code. Wouldn't it be simpler to have two lists, one > > for non-partial and another for partial paths? I went to check the > > original discussion, and this design was indeed considered [1] -- but > > the idea was discarded because using the list index would lead to > > simpler code. However, now that we have pruning it seems to me that > > using the index isn't simpler anymore. Should we revisit this now? > > I don't think having two Lists and/or two AppendState arrays would > make the pruning code anymore simple. All the pruning code in > execPartition.c would need to determine the index within the partial > or non-partial subnode array, and also communicate which array it > means. That code did take me a while to get right and be readable > too, I don't really want to have to change it again. I really don't > think it would look quite as simple as it does today either, so -1 > from me for changing this. Got it. -- Álvaro Herrera https://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services