Re: ERROR: ORDER/GROUP BY expression not found in targetlist
| От | Amit Kapila |
|---|---|
| Тема | Re: ERROR: ORDER/GROUP BY expression not found in targetlist |
| Дата | |
| Msg-id | CAA4eK1+HMTODErjj4qFP+NoLt7k9Od69PPQuxyXSRYtgypDpnw@mail.gmail.com обсуждение исходный текст |
| Ответ на | Re: ERROR: ORDER/GROUP BY expression not found in targetlist (Tom Lane <tgl@sss.pgh.pa.us>) |
| Ответы |
Re: ERROR: ORDER/GROUP BY expression not found in targetlist
(Amit Kapila <amit.kapila16@gmail.com>)
|
| Список | pgsql-hackers |
On Mon, Jun 13, 2016 at 8:11 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Kapila <amit.kapila@enterprisedb.com> writes:
> > On Mon, Jun 13, 2016 at 7:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I think the real question here is why the code removed by 04ae11f62
> >> was wrong. It was unsafe to use apply_projection_to_path, certainly,
> >> but using create_projection_path directly would have avoided the
> >> stated problem. And it's very unclear that this new patch doesn't
> >> bring back that bug in a different place.
>
> > This new patch still doesn't seem to be right, but it won't bring back the
> > original problem because apply_projection_to_path will be only done if
> > grouped_rel is parallel_safe which means it doesn't have any
> > parallel-unsafe or parallel-restricted clause in quals or target list.
>
> The problem cited in 04ae11f62's commit message is that
> apply_projection_to_path would overwrite the paths' pathtargets in-place,
> causing problems if they'd been used for other purposes elsewhere.
>
> Amit Kapila <amit.kapila@enterprisedb.com> writes:
> > On Mon, Jun 13, 2016 at 7:51 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I think the real question here is why the code removed by 04ae11f62
> >> was wrong. It was unsafe to use apply_projection_to_path, certainly,
> >> but using create_projection_path directly would have avoided the
> >> stated problem. And it's very unclear that this new patch doesn't
> >> bring back that bug in a different place.
>
> > This new patch still doesn't seem to be right, but it won't bring back the
> > original problem because apply_projection_to_path will be only done if
> > grouped_rel is parallel_safe which means it doesn't have any
> > parallel-unsafe or parallel-restricted clause in quals or target list.
>
> The problem cited in 04ae11f62's commit message is that
> apply_projection_to_path would overwrite the paths' pathtargets in-place,
> causing problems if they'd been used for other purposes elsewhere.
>
The main reason for removal of that code is that because there was no check there to prevent assigning of parallel-restricted clauses to pathtarget of partial paths. I think the same is indicated in commit message as well, if we focus on below part of commit message:
"especially because this code has no check that the PathTarget is in fact parallel-safe."
Due to above reason, I don't see how the suggestion given by you to avoid the problem by using create_projection_path can work.
> I do
> not share your confidence that using apply_projection_to_path within
> create_grouping_paths is free of such a hazard.
>
> not share your confidence that using apply_projection_to_path within
> create_grouping_paths is free of such a hazard.
>
Fair enough, let me try to explain the problem and someways to solve it in some more detail. The main thing that got missed by me in the patch related to commit-04ae11f62 is that the partial path list of rel also needs to have a scanjoin_target. I was under assumption that create_grouping_paths will take care of assigning appropriate Path targets for the parallel paths generated by it. If we see, create_grouping_paths() do take care of adding the appropriate path targets for the paths generated by that function. However, it doesn't do anything for partial paths. The patch sent by me yesterday [1] which was trying to assign partial_grouping_target to partial paths won't be the right fix, because (a) partial_grouping_target includes Aggregates (refer make_partialgroup_input_target) which we don't want for partial paths; (b) it is formed using grouping_target passed in function create_grouping_paths() whereas we need the path target formed from final_target for partial paths (as partial paths are scanjoin relations) as is done for main path list (in grouping_planner(), /* Forcibly apply that target to all the Paths for the scan/join rel.*/). Now, I think we have following ways to solve it:
(a) check whether the scanjoin_target contains any parallel-restricted clause before applying the same to partial path list in grouping_planner. However it could lead to duplicate checks in some cases for parallel-restricted clause, once in apply_projection_to_path() for main pathlist and then again before applying to partial paths. I think we can avoid that by having an additional parameter in apply_projection_to_path() which can indicate if the check for parallel-safety is done inside that function and what is the result of same.
(b) generate the appropriate scanjoin_target for partial path list in create_grouping_paths using final_target. However I think this might lead to some duplicate code in create_grouping_paths() as we might have to some thing similar to what we have done in grouping_planner for generating such a target. I think if we want we can pass scanjoin_target to create_grouping_paths() as well. Again, we have to check for parallel-safety for scanjoin_target before applying it to partial paths, but we need to do that only when grouped_rel is considered parallel-safe.
Thoughts?
В списке pgsql-hackers по дате отправления:
Следующее
От: Kyotaro HORIGUCHIДата:
Сообщение: Re: [BUG] pg_basebackup from disconnected standby fails