From: Antonin Houska <ah@cybertec.at>
To: James Coleman <jtc331@gmail.com>
cc: Tomas Vondra <tomas.vondra@enterprisedb.com>,
    vignesh C <vignesh21@gmail.com>, Andres Freund <andres@anarazel.de>,
    Robert Haas <robertmhaas@gmail.com>,
    pgsql-hackers <pgsql-hackers@postgresql.org>,
    Amit Kapila <amit.kapila16@gmail.com>, Tom Lane <tgl@sss.pgh.pa.us>
Subject: Re: Parallelize correlated subqueries that execute within each worker
In-reply-to: <CAAaqYe9V+2TTwGDUu12NLu1tDf0SnwvYnTOrKz=uTh6cLx3McA@mail.gmail.com>
References: <CAAaqYe_x1u3V4uPiv=dJ=k2EJ7txhdq6yexJDkYZ1x1pu0QwcQ@mail.gmail.com> <CA+TgmobtK2OnhNaKbq8Q+k217mCUdpLFeMXTjxr6QZMG3KE5Gw@mail.gmail.com> <CAAaqYe-FYVq=CSZu8VtKi0fx8Zdi6fzyzEP2B9iK2cUgY8xGjA@mail.gmail.com> <CA+TgmoaAPfPcQx0uYLusU4A+Vm5Cr2F8irxo=xf8BQ-97YyZ7A@mail.gmail.com> <CAAaqYe8HLwTGJC2jPZ8noiv2SRSSvS3zSjQQMmeUqNiY4Uw_dw@mail.gmail.com> <CA+TgmoYXm2NCLt1nikWfYj1_r3=fsoNCHCtDVdN7X1uX_xuXgw@mail.gmail.com> <CAAaqYe8m0DHUWk7gLKb_C4abTD4nMkU26ErE=ahow4zNMZbzPQ@mail.gmail.com> <20220322004805.vwmgjp62tq3mj2oq@alap3.anarazel.de> <CAAaqYe9hJ=GzcaVSH82e8o6edr3bkYkauhk_yLiGVbF=8xss1w@mail.gmail.com> <CALDaNm0vp-xLtVEwFHBjTP38ufNN1e-b6J17nDsUhNNiqZ8iHw@mail.gmail.com> <1818a285-59b9-e028-c5a8-539f715b9371@enterprisedb.com> <CAAaqYe8=wzXDB5fpO8geK+DiTdz9HaDBRy-2JOiAPC-8vcnXDA@mail.gmail.com> <CAAaqYe_ByNJxaCYqN6uKKeyTpJWkEXBoYrCjvAQGNrVE5ZEYzg@mail.gmail.com> <CAAaqYe9V+2TTwGDUu12NLu1tDf0SnwvYnTOrKz=uTh6cLx3McA@mail.gmail.com>
Comments: In-reply-to James Coleman <jtc331@gmail.com>
   message dated "Mon, 23 Jan 2023 09:00:31 -0500."
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <20282.1675701654.1@antos>
Content-Transfer-Encoding: quoted-printable
Date: Mon, 06 Feb 2023 17:40:55 +0100
Message-ID: <20283.1675701655@antos>
Archived-At: <https://www.postgresql.org/message-id/20283.1675701655%40antos>
Precedence: bulk

James Coleman <jtc331@gmail.com> wrote:

> On Sat, Jan 21, 2023 at 10:07 PM James Coleman <jtc331@gmail.com> wrote:
> > ...
> > While working through Tomas's comment about a conditional in the
> > max_parallel_hazard_waker being guaranteed true I realized that in the
> > current version of the patch the safe_param_ids tracking in
> > is_parallel_safe isn't actually needed any longer. That seemed
> > suspicious, and so I started digging, and I found out that in the
> > current approach all of the tests pass with only the changes in
> > clauses.c. I don't believe that the other changes aren't needed;
> > rather I believe there isn't yet a test case exercising them, but I
> > realize that means I can't prove they're needed. I spent some time
> > poking at this, but at least with my current level of imagination I
> > haven't stumbled across a query that would exercise these checks.
> =

> I played with this a good bit more yesterday, I'm now a good bit more
> confident this is correct. I've cleaned up the patch; see attached for
> v7.
> =

> Here's some of my thought process:
> The comments in src/include/nodes/pathnodes.h:2953 tell us that
> PARAM_EXEC params are used to pass values around from one plan node to
> another in the following ways:
> 1. Values down into subqueries (for outer references in subqueries)
> 2. Up out of subqueries (for the results of a subplan)
> 3. From a NestLoop plan node into its inner relation (when the inner
> scan is parameterized with values from the outer relation)
> =

> Case (2) is already known to be safe (we currently add these params to
> safe_param_ids in max_parallel_hazard_walker when we encounter a
> SubPlan node).
> =

> I also believe case (3) is already handled. We don't build partial
> paths for joins when joinrel->lateral_relids is non-empty, and join
> order calculations already require that parameterization here go the
> correct way (i.e., inner depends on outer rather than the other way
> around).
> =

> That leaves us with only case (1) to consider in this patch. Another
> way of saying this is that this is really the only thing the
> safe_param_ids tracking is guarding against. For params passed down
> into subqueries we can further distinguish between init plans and
> "regular" subplans. We already know that params from init plans are
> safe (at the right level). So we're concerned here with a way to know
> if the params passed to subplans are safe. We already track required
> rels in ParamPathInfo, so it's fairly simple to do this test.
> =

> Which this patch we do in fact now see (as expected) rels with
> non-empty lateral_relids showing up in generate_[useful_]gather_paths.
> And the partial paths can now have non-empty required outer rels. I'm
> not able to come up with a plan that would actually be caught by those
> checks; I theorize that because of the few places we actually call
> generate_[useful_]gather_paths we are in practice already excluding
> those, but for now I've left these as a conditional rather than an
> assertion because it seems like the kind of guard we'd want to ensure
> those methods are safe.

Maybe we can later (in separate patches) relax the restrictions imposed on
partial path creation a little bit, so that more parameterized partial pat=
hs
are created.

One particular case that should be rejected by your checks is a partial in=
dex
path, which can be parameterized, but I couldn't construct a query that ma=
kes
your checks fire. Maybe the reason is that a parameterized index path is
mostly used on the inner side of a NL join, however no partial path can be
used there. (The problem is that each worker evaluating the NL join would =
only
see a subset of the inner relation, which whould lead to incorrect results=
.)

So I'd also choose conditions rather than assert statements.


Following are my (minor) findings:

In generate_gather_paths() you added this test

        /*
         * Delay gather path creation until the level in the join tree whe=
re all
         * params used in a worker are generated within that worker.
         */
        if (!bms_is_subset(required_outer, rel->relids))
                return;

but I'm not sure if required_outer can contain anything of rel->relids. Ho=
w
about using bms_is_empty(required) outer, or even this?

	if (required_outer)
		return;

Similarly,

        /* We can't pass params to workers. */
        if (!bms_is_subset(PATH_REQ_OUTER(cheapest_partial_path), rel->rel=
ids))

might look like

        if (!bms_is_empty(PATH_REQ_OUTER(cheapest_partial_path)))

or

        if (PATH_REQ_OUTER(cheapest_partial_path))

In particular, build_index_paths() does the following when setting
outer_relids (which eventually becomes (path->param_info->ppi_req_outer):

        /* Enforce convention that outer_relids is exactly NULL if empty *=
/
        if (bms_is_empty(outer_relids))
                outer_relids =3D NULL;


Another question is whether in this call

        simple_gather_path =3D (Path *)
                create_gather_path(root, rel, cheapest_partial_path, rel->=
reltarget,
                                                   required_outer, rowsp);

required_outer should be passed to create_gather_path(). Shouldn't it rath=
er
be PATH_REQ_OUTER(cheapest_partial_path) that you test just above? Again,
build_index_paths() initializes outer_relids this way

        outer_relids =3D bms_copy(rel->lateral_relids);

but then it may add some more relations:

        /* OK to include this clause */
        index_clauses =3D lappend(index_clauses, iclause);
        outer_relids =3D bms_add_members(outer_relids,
                                                           rinfo->clause_r=
elids);

So I think that PATH_REQ_OUTER(cheapest_partial_path) in
generate_gather_paths() can eventually contain more relations than
required_outer, and therefore it's safer to check the first.


Similar comments might apply to generate_useful_gather_paths(). Here I als=
o
suggest to move this test

        /* We can't pass params to workers. */
        if (!bms_is_subset(PATH_REQ_OUTER(subpath), rel->relids))
                continue;

to the top of the loop because it's relatively cheap.

-- =

Antonin Houska
Web: https://www.cybertec-postgresql.com