Обсуждение: XX000: iso-8859-1 type of jsonb container.

> On Wed, Apr 07, 2021 at 10:59:31AM +0000, Poot, Bas (B.J.) wrote:
>
> Finally! I have a testcase for you guys. This is my query to generate the data.
> select *
> into tmp_bug2
> from (
> select 'thing' as  logical_name
> , 'thing' as display_name
> , 'thing' as operation
> , '{"unit": "Our special Unit", "some.place": "placename", "place_of_birth": "Over the rainbow", "How_secret_is_it":
"Level1", "Does.it.shoot": "YES!"}'::jsonb as filtur
 
> from generate_series(1, 302443)
> ) t1;
>
> And this is the query that generates the error.
>
> select
> display_name
> ,'' as x
> ,filtur
> ,jsonb_each_text(filtur) as x
> ,to_jsonb(jsonb_each_text(filtur)) as frows
> ,array(SELECT jsonb_object_keys(filtur)) as objectkeys
> from tmp_bug2
> order by logical_name;
>
> Notice that when you generate not 302443 but 302442 rows, it works perfectly fine.

Thanks for posting the test case, I can reproduce it on the master
branch as well (also without the second call to jsonb_each_text in line
with to_jsonb). Interesting, it looks like for unclear to me reasons the
argument, evaluated in ExecMakeFunctionResultSet for jsonb_each_text,
contains value from the previous attribute, not jsonb. This makes
iteratorFromContainer complain because both array & object flags are set
in the header. I'll try to investigate, unless someone else will be
faster.

> On Wed, Apr 07, 2021 at 08:11:05PM +0200, Dmitry Dolgov wrote:
> > On Wed, Apr 07, 2021 at 10:59:31AM +0000, Poot, Bas (B.J.) wrote:
> >
> > Finally! I have a testcase for you guys. This is my query to generate the data.
> > select *
> > into tmp_bug2
> > from (
> > select 'thing' as  logical_name
> > , 'thing' as display_name
> > , 'thing' as operation
> > , '{"unit": "Our special Unit", "some.place": "placename", "place_of_birth": "Over the rainbow",
"How_secret_is_it":"Level 1", "Does.it.shoot": "YES!"}'::jsonb as filtur
 
> > from generate_series(1, 302443)
> > ) t1;
> >
> > And this is the query that generates the error.
> >
> > select
> > display_name
> > ,'' as x
> > ,filtur
> > ,jsonb_each_text(filtur) as x
> > ,to_jsonb(jsonb_each_text(filtur)) as frows
> > ,array(SELECT jsonb_object_keys(filtur)) as objectkeys
> > from tmp_bug2
> > order by logical_name;
> >
> > Notice that when you generate not 302443 but 302442 rows, it works perfectly fine.
>
> Thanks for posting the test case, I can reproduce it on the master
> branch as well (also without the second call to jsonb_each_text in line
> with to_jsonb). Interesting, it looks like for unclear to me reasons the
> argument, evaluated in ExecMakeFunctionResultSet for jsonb_each_text,
> contains value from the previous attribute, not jsonb. This makes
> iteratorFromContainer complain because both array & object flags are set
> in the header. I'll try to investigate, unless someone else will be
> faster.

Here is what I've found so far:

* It seems that technically the regression was introduced in
  ea190ed14b, but not directly, via using gather paths in more
  situations.

* The direct problem is that JsonbIteratorInit is confused by the
  argument, because it contains both headers for array and object set.
  From what I understood this confusion stems from
  ExecMakeFunctionResultSet taking wrong value from the slot. Hacking it
  to use different attnum in this case makes it work.

* The explanation for this could be that target list from where attnum
  is taken and slot values have different order of elements. This in
  turn comes out of grouping planner trying to isolate SRF and SRF-free
  targets and as a result changing the order of elements in
  final_target. The final_target is then passed into
  create_ordered_paths and applied via apply_projection_to_path, but
  somehow goes in disagreement with what is used while creating the slot
  with values.

If this analysis is correct, I'm not sure yet what would be the best
course of action to address the problem, need to think a bit more. But
probably others have suggestions or comments?

> On Tue, Apr 13, 2021 at 06:15:00PM +0200, Dmitry Dolgov wrote:
> > On Wed, Apr 07, 2021 at 08:11:05PM +0200, Dmitry Dolgov wrote:
> > > On Wed, Apr 07, 2021 at 10:59:31AM +0000, Poot, Bas (B.J.) wrote:
> > >
> > > Finally! I have a testcase for you guys. This is my query to generate the data.
> > > select *
> > > into tmp_bug2
> > > from (
> > > select 'thing' as  logical_name
> > > , 'thing' as display_name
> > > , 'thing' as operation
> > > , '{"unit": "Our special Unit", "some.place": "placename", "place_of_birth": "Over the rainbow",
"How_secret_is_it":"Level 1", "Does.it.shoot": "YES!"}'::jsonb as filtur
 
> > > from generate_series(1, 302443)
> > > ) t1;
> > >
> > > And this is the query that generates the error.
> > >
> > > select
> > > display_name
> > > ,'' as x
> > > ,filtur
> > > ,jsonb_each_text(filtur) as x
> > > ,to_jsonb(jsonb_each_text(filtur)) as frows
> > > ,array(SELECT jsonb_object_keys(filtur)) as objectkeys
> > > from tmp_bug2
> > > order by logical_name;
> > >
> > > Notice that when you generate not 302443 but 302442 rows, it works perfectly fine.
> >
> > Thanks for posting the test case, I can reproduce it on the master
> > branch as well (also without the second call to jsonb_each_text in line
> > with to_jsonb). Interesting, it looks like for unclear to me reasons the
> > argument, evaluated in ExecMakeFunctionResultSet for jsonb_each_text,
> > contains value from the previous attribute, not jsonb. This makes
> > iteratorFromContainer complain because both array & object flags are set
> > in the header. I'll try to investigate, unless someone else will be
> > faster.
> 
> Here is what I've found so far:
> 
> * It seems that technically the regression was introduced in
>   ea190ed14b, but not directly, via using gather paths in more
>   situations.
> 
> * The direct problem is that JsonbIteratorInit is confused by the
>   argument, because it contains both headers for array and object set.
>   From what I understood this confusion stems from
>   ExecMakeFunctionResultSet taking wrong value from the slot. Hacking it
>   to use different attnum in this case makes it work.
> 
> * The explanation for this could be that target list from where attnum
>   is taken and slot values have different order of elements. This in
>   turn comes out of grouping planner trying to isolate SRF and SRF-free
>   targets and as a result changing the order of elements in
>   final_target. The final_target is then passed into
>   create_ordered_paths and applied via apply_projection_to_path, but
>   somehow goes in disagreement with what is used while creating the slot
>   with values.
> 
> If this analysis is correct, I'm not sure yet what would be the best
> course of action to address the problem, need to think a bit more. But
> probably others have suggestions or comments?

I couldn't find any other feasible explanations, and have come to a
conclusion that this happens when a projection is applied to a
GatherMerge path. As it's a projection capable path, no new projection
is created and target list is changed in place. In the subpath target
list ordering is different because of query ordering, and I don't see
where it all comes together during execution. Funny enough even explain
shows that the final plan passes a wrong values to jsonb_each_text.

If I make GatherMerge non projection capable it fixes this particular
case and changes only one test in select_parallel (seems like a minor
plan changes). But I have not enough experience with this code to say if
it's a good or bad idea.

> On Fri, Apr 23, 2021 at 05:15:05PM +0200, Dmitry Dolgov wrote:
> > On Tue, Apr 13, 2021 at 06:15:00PM +0200, Dmitry Dolgov wrote:
> > > On Wed, Apr 07, 2021 at 08:11:05PM +0200, Dmitry Dolgov wrote:
> > > > On Wed, Apr 07, 2021 at 10:59:31AM +0000, Poot, Bas (B.J.) wrote:
> > > >
> > > > Finally! I have a testcase for you guys. This is my query to generate the data.
> > > > select *
> > > > into tmp_bug2
> > > > from (
> > > > select 'thing' as  logical_name
> > > > , 'thing' as display_name
> > > > , 'thing' as operation
> > > > , '{"unit": "Our special Unit", "some.place": "placename", "place_of_birth": "Over the rainbow",
"How_secret_is_it":"Level 1", "Does.it.shoot": "YES!"}'::jsonb as filtur
 
> > > > from generate_series(1, 302443)
> > > > ) t1;
> > > >
> > > > And this is the query that generates the error.
> > > >
> > > > select
> > > > display_name
> > > > ,'' as x
> > > > ,filtur
> > > > ,jsonb_each_text(filtur) as x
> > > > ,to_jsonb(jsonb_each_text(filtur)) as frows
> > > > ,array(SELECT jsonb_object_keys(filtur)) as objectkeys
> > > > from tmp_bug2
> > > > order by logical_name;
> > > >
> > > > Notice that when you generate not 302443 but 302442 rows, it works perfectly fine.
> > >
> > > Thanks for posting the test case, I can reproduce it on the master
> > > branch as well (also without the second call to jsonb_each_text in line
> > > with to_jsonb). Interesting, it looks like for unclear to me reasons the
> > > argument, evaluated in ExecMakeFunctionResultSet for jsonb_each_text,
> > > contains value from the previous attribute, not jsonb. This makes
> > > iteratorFromContainer complain because both array & object flags are set
> > > in the header. I'll try to investigate, unless someone else will be
> > > faster.
> > 
> > Here is what I've found so far:
> > 
> > * It seems that technically the regression was introduced in
> >   ea190ed14b, but not directly, via using gather paths in more
> >   situations.
> > 
> > * The direct problem is that JsonbIteratorInit is confused by the
> >   argument, because it contains both headers for array and object set.
> >   From what I understood this confusion stems from
> >   ExecMakeFunctionResultSet taking wrong value from the slot. Hacking it
> >   to use different attnum in this case makes it work.
> > 
> > * The explanation for this could be that target list from where attnum
> >   is taken and slot values have different order of elements. This in
> >   turn comes out of grouping planner trying to isolate SRF and SRF-free
> >   targets and as a result changing the order of elements in
> >   final_target. The final_target is then passed into
> >   create_ordered_paths and applied via apply_projection_to_path, but
> >   somehow goes in disagreement with what is used while creating the slot
> >   with values.
> > 
> > If this analysis is correct, I'm not sure yet what would be the best
> > course of action to address the problem, need to think a bit more. But
> > probably others have suggestions or comments?
> 
> I couldn't find any other feasible explanations, and have come to a
> conclusion that this happens when a projection is applied to a
> GatherMerge path. As it's a projection capable path, no new projection
> is created and target list is changed in place. In the subpath target
> list ordering is different because of query ordering, and I don't see
> where it all comes together during execution. Funny enough even explain
> shows that the final plan passes a wrong values to jsonb_each_text.
> 
> If I make GatherMerge non projection capable it fixes this particular
> case and changes only one test in select_parallel (seems like a minor
> plan changes). But I have not enough experience with this code to say if
> it's a good or bad idea.

Almost forgot about this one. It seems that the issue still could be
reproduced on the latest master branch, so probably worth an open item.

> On Wed, May 26, 2021 at 01:52:07PM +0000, Poot, Bas (B.J.) wrote:
>
> thank you for not forgetting 👍
> 
> I read about the postgres 14 beta and was afraid the bug was indeed still in there..
> 
> Is there anything else I can do to help you solve this?

Theoretically nothing else, you've already done a lot by providing such
a good reproducing script :) I just need to find someone with better
understanding of GatherMerge part to confirm my conclusions or suggest
something better.

Dmitry Dolgov <9erthalion6@gmail.com> writes:
> I couldn't find any other feasible explanations, and have come to a
> conclusion that this happens when a projection is applied to a
> GatherMerge path. As it's a projection capable path, no new projection
> is created and target list is changed in place. In the subpath target
> list ordering is different because of query ordering, and I don't see
> where it all comes together during execution. Funny enough even explain
> shows that the final plan passes a wrong values to jsonb_each_text.

I dug into this, and found that the proximate problem is that we're
generating a Sort node in which the output column order is different
from the input.  If Sort could project, that'd be fine, but of course
it doesn't.  (I wonder if there should be some more assertions in
setrefs.c to try to catch plans that are broken like that.)

Of course, it's not like the planner has never heard of that restriction.
The problem occurs because what arrives at createplan.c looks like

    ProjectionPath
        ProjectionPath
            SortPath
                SeqScanPath

that is, we've got *two* redundant ProjectionPaths, and the logic in
createplan.c that is supposed to decide whether we can skip generating
a projecting Result node for a ProjectionPath gets confused and
decides we don't need any Result, rather than deciding we need one
Result.

Maybe we could work around that, but having two stacked ProjectionPaths
is just silly, so the attached patch deals with this problem by making
sure we don't do that.

The place where that happens in the example reported in this thread is
apply_projection_to_path, which somebody kluged up to the point where
it'd create this situation just below a Gather/GatherMerge node.
So I first tried to fix it there, and for safety added an Assert to
create_projection_path saying that its input wasn't a ProjectionPath.

The Assert blew up the regression tests.  The same thing is happening
in other places, and only by luck have we not noticed any bad effects.

So the attached fixes it by stripping any input ProjectionPath in
create_projection_path, instead.

I'm not sure about creating a test case.  The case reported here is
far too expensive to use in the regression tests.

            regards, tom lane

diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index b248b038e0..6c6bde04f6 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -2632,7 +2632,22 @@ create_projection_path(PlannerInfo *root,
                        PathTarget *target)
 {
     ProjectionPath *pathnode = makeNode(ProjectionPath);
-    PathTarget *oldtarget = subpath->pathtarget;
+    PathTarget *oldtarget;
+
+    /*
+     * We mustn't put a ProjectionPath directly above another; it's useless
+     * and will confuse createplan.c.  Rather than making sure all callers
+     * know that, let's implement it here, by stripping off any ProjectionPath
+     * in what we're given.  Given this rule, there won't be more than one.
+     */
+    if (IsA(subpath, ProjectionPath))
+    {
+        ProjectionPath *subpp = (ProjectionPath *) subpath;
+
+        Assert(subpp->path.parent == rel);
+        subpath = subpp->subpath;
+        Assert(!IsA(subpath, ProjectionPath));
+    }

     pathnode->path.pathtype = T_Result;
     pathnode->path.parent = rel;
@@ -2658,6 +2673,7 @@ create_projection_path(PlannerInfo *root,
      * Note: in the latter case, create_projection_plan has to recheck our
      * conclusion; see comments therein.
      */
+    oldtarget = subpath->pathtarget;
     if (is_projection_capable_path(subpath) ||
         equal(oldtarget->exprs, target->exprs))
     {

I wrote:
> I'm not sure about creating a test case.  The case reported here is
> far too expensive to use in the regression tests.

Ah, we can do it in select_parallel, which reduces all the parallel
costs to zero to favor these sorts of plans.

This test case only fails back to v13, as does the original example.
I suspect we should back-patch the code change further though,
since create_projection_plan will be just as confused by such cases
in earlier branches.

            regards, tom lane

diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c
index b248b038e0..9ce5f95e3b 100644
--- a/src/backend/optimizer/util/pathnode.c
+++ b/src/backend/optimizer/util/pathnode.c
@@ -2632,7 +2632,23 @@ create_projection_path(PlannerInfo *root,
                        PathTarget *target)
 {
     ProjectionPath *pathnode = makeNode(ProjectionPath);
-    PathTarget *oldtarget = subpath->pathtarget;
+    PathTarget *oldtarget;
+
+    /*
+     * We mustn't put a ProjectionPath directly above another; it's useless
+     * and will confuse create_projection_plan.  Rather than making sure all
+     * callers handle that, let's implement it here, by stripping off any
+     * ProjectionPath in what we're given.  Given this rule, there won't be
+     * more than one.
+     */
+    if (IsA(subpath, ProjectionPath))
+    {
+        ProjectionPath *subpp = (ProjectionPath *) subpath;
+
+        Assert(subpp->path.parent == rel);
+        subpath = subpp->subpath;
+        Assert(!IsA(subpath, ProjectionPath));
+    }

     pathnode->path.pathtype = T_Result;
     pathnode->path.parent = rel;
@@ -2658,6 +2674,7 @@ create_projection_path(PlannerInfo *root,
      * Note: in the latter case, create_projection_plan has to recheck our
      * conclusion; see comments therein.
      */
+    oldtarget = subpath->pathtarget;
     if (is_projection_capable_path(subpath) ||
         equal(oldtarget->exprs, target->exprs))
     {
diff --git a/src/test/regress/expected/select_parallel.out b/src/test/regress/expected/select_parallel.out
index 05ebcb284a..4ea1aa7dfd 100644
--- a/src/test/regress/expected/select_parallel.out
+++ b/src/test/regress/expected/select_parallel.out
@@ -1126,6 +1126,29 @@ ORDER BY 1, 2, 3;
 ------------------------------+---------------------------+-------------+--------------
 (0 rows)

+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT generate_series(1, two), array(select generate_series(1, two))
+  FROM tenk1 ORDER BY tenthous;
+                              QUERY PLAN
+----------------------------------------------------------------------
+ ProjectSet
+   Output: generate_series(1, tenk1.two), (SubPlan 1), tenk1.tenthous
+   ->  Gather Merge
+         Output: tenk1.two, tenk1.tenthous
+         Workers Planned: 4
+         ->  Result
+               Output: tenk1.two, tenk1.tenthous
+               ->  Sort
+                     Output: tenk1.tenthous, tenk1.two
+                     Sort Key: tenk1.tenthous
+                     ->  Parallel Seq Scan on public.tenk1
+                           Output: tenk1.tenthous, tenk1.two
+   SubPlan 1
+     ->  ProjectSet
+           Output: generate_series(1, tenk1.two)
+           ->  Result
+(16 rows)
+
 -- test passing expanded-value representations to workers
 CREATE FUNCTION make_some_array(int,int) returns int[] as
 $$declare x int[];
diff --git a/src/test/regress/sql/select_parallel.sql b/src/test/regress/sql/select_parallel.sql
index d31e290ec2..f924731248 100644
--- a/src/test/regress/sql/select_parallel.sql
+++ b/src/test/regress/sql/select_parallel.sql
@@ -429,6 +429,10 @@ ORDER BY 1;
 SELECT * FROM information_schema.foreign_data_wrapper_options
 ORDER BY 1, 2, 3;

+EXPLAIN (VERBOSE, COSTS OFF)
+SELECT generate_series(1, two), array(select generate_series(1, two))
+  FROM tenk1 ORDER BY tenthous;
+
 -- test passing expanded-value representations to workers
 CREATE FUNCTION make_some_array(int,int) returns int[] as
 $$declare x int[];

> On Sun, May 30, 2021 at 08:59:28PM -0400, Tom Lane wrote:
> 
> Of course, it's not like the planner has never heard of that restriction.
> The problem occurs because what arrives at createplan.c looks like
> 
>     ProjectionPath
>         ProjectionPath
>             SortPath
>                 SeqScanPath
> 
> that is, we've got *two* redundant ProjectionPaths, and the logic in
> createplan.c that is supposed to decide whether we can skip generating
> a projecting Result node for a ProjectionPath gets confused and
> decides we don't need any Result, rather than deciding we need one
> Result.
> 
> Maybe we could work around that, but having two stacked ProjectionPaths
> is just silly, so the attached patch deals with this problem by making
> sure we don't do that.
> 
> The place where that happens in the example reported in this thread is
> apply_projection_to_path, which somebody kluged up to the point where
> it'd create this situation just below a Gather/GatherMerge node.
> So I first tried to fix it there, and for safety added an Assert to
> create_projection_path saying that its input wasn't a ProjectionPath.
> 
> The Assert blew up the regression tests.  The same thing is happening
> in other places, and only by luck have we not noticed any bad effects.
> 
> So the attached fixes it by stripping any input ProjectionPath in
> create_projection_path, instead.
 
Thanks for looking into this. I don't see any problems with the
suggested patch, so +1.

> This test case only fails back to v13, as does the original example.
> I suspect we should back-patch the code change further though,
> since create_projection_plan will be just as confused by such cases
> in earlier branches.

That would be my assumption as well.

Dmitry Dolgov <9erthalion6@gmail.com> writes:
> On Sun, May 30, 2021 at 08:59:28PM -0400, Tom Lane wrote:
>> This test case only fails back to v13, as does the original example.
>> I suspect we should back-patch the code change further though,
>> since create_projection_plan will be just as confused by such cases
>> in earlier branches.

> That would be my assumption as well.

Done now.  I spent a little bit of time looking for a test case
that'd fail in the prior branches, without success.  I think
it's highly probable that there is one, but I didn't want to
spend any more time here.

            regards, tom lane