Re: XX000: iso-8859-1 type of jsonb container. - Mailing list pgsql-bugs

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))
     {