Oversight in reparameterize_path_by_child leading to executor crash

Richard Guo <guofenglinux@gmail.com> writes:

In this case what we need to do is to adjust the TableSampleClause to
refer to the correct child relations. We can do that with the help of
adjust_appendrel_attrs_multilevel(). One problem is that the
TableSampleClause is stored in RangeTblEntry, and it does not seem like
a good practice to alter RangeTblEntry in this place.

Ugh. That's why we didn't think to adjust it, obviously. You are right
that we can't just modify the RTE on the fly, since it's shared with
other non-parameterized paths.

So what I'm thinking is that maybe we can add a new type of path, named
SampleScanPath, to have the TableSampleClause per path. Then we can
safely reparameterize the TableSampleClause as needed for each
SampleScanPath. That's what the attached patch does.

Alternatively, could we postpone the reparameterization until
createplan.c? Having to build reparameterized expressions we might
not use seems expensive, and it would also contribute to the memory
bloat being complained of in nearby threads.

There are some other plan types that do not have a separate path type
but may have lateral references in expressions stored in RangeTblEntry,
such as FunctionScan, TableFuncScan and ValuesScan. But it's not clear
to me if there are such problems with them too.

Hmm, well, not for partition-wise join anyway.

regards, tom lane

On Tue, Aug 1, 2023 at 9:20 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Richard Guo <guofenglinux@gmail.com> writes:

So what I'm thinking is that maybe we can add a new type of path, named
SampleScanPath, to have the TableSampleClause per path. Then we can
safely reparameterize the TableSampleClause as needed for each
SampleScanPath. That's what the attached patch does.

Alternatively, could we postpone the reparameterization until
createplan.c? Having to build reparameterized expressions we might
not use seems expensive, and it would also contribute to the memory
bloat being complained of in nearby threads.

I did think about this option but figured out that it seems beyond the
scope of just fixing SampleScan. But if we want to optimize the
reparameterization mechanism besides fixing this crash, I think this
option is much better. I drafted a patch as attached.

Thanks
Richard

On Tue, Aug 1, 2023 at 6:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alternatively, could we postpone the reparameterization until
createplan.c? Having to build reparameterized expressions we might
not use seems expensive, and it would also contribute to the memory
bloat being complained of in nearby threads.

As long as the translation happens only once, it should be fine. It's
the repeated translations which should be avoided.

--
Best Wishes,
Ashutosh Bapat

On Thu, Aug 3, 2023 at 12:38 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

On Tue, Aug 1, 2023 at 6:50 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

Alternatively, could we postpone the reparameterization until
createplan.c? Having to build reparameterized expressions we might
not use seems expensive, and it would also contribute to the memory
bloat being complained of in nearby threads.

As long as the translation happens only once, it should be fine. It's
the repeated translations which should be avoided.

Sometimes it would help to avoid the translation at all if we postpone
the reparameterization until createplan.c, such as in the case that a
non-parameterized path wins at last. For instance, for the query below

regression=# explain (costs off)
select * from prt1 t1 join prt1 t2 on t1.a = t2.a;
QUERY PLAN
--------------------------------------------
Append
-> Hash Join
Hash Cond: (t1_1.a = t2_1.a)
-> Seq Scan on prt1_p1 t1_1
-> Hash
-> Seq Scan on prt1_p1 t2_1
-> Hash Join
Hash Cond: (t1_2.a = t2_2.a)
-> Seq Scan on prt1_p2 t1_2
-> Hash
-> Seq Scan on prt1_p2 t2_2
-> Hash Join
Hash Cond: (t1_3.a = t2_3.a)
-> Seq Scan on prt1_p3 t1_3
-> Hash
-> Seq Scan on prt1_p3 t2_3
(16 rows)

Our current code would reparameterize each inner paths although at last
we choose the non-parameterized paths. If we do the reparameterization
in createplan.c, we would be able to avoid all the translations.

Thanks
Richard

On Thu, Aug 3, 2023 at 4:14 PM Richard Guo <guofenglinux@gmail.com> wrote:

On Thu, Aug 3, 2023 at 12:38 PM Ashutosh Bapat <
ashutosh.bapat.oss@gmail.com> wrote:

Sometimes it would help to avoid the translation at all if we postpone
the reparameterization until createplan.c, such as in the case that a
non-parameterized path wins at last. For instance, for the query below

regression=# explain (costs off)
select * from prt1 t1 join prt1 t2 on t1.a = t2.a;
QUERY PLAN
--------------------------------------------
Append
-> Hash Join
Hash Cond: (t1_1.a = t2_1.a)
-> Seq Scan on prt1_p1 t1_1
-> Hash
-> Seq Scan on prt1_p1 t2_1
-> Hash Join
Hash Cond: (t1_2.a = t2_2.a)
-> Seq Scan on prt1_p2 t1_2
-> Hash
-> Seq Scan on prt1_p2 t2_2
-> Hash Join
Hash Cond: (t1_3.a = t2_3.a)
-> Seq Scan on prt1_p3 t1_3
-> Hash
-> Seq Scan on prt1_p3 t2_3
(16 rows)

Our current code would reparameterize each inner paths although at last
we choose the non-parameterized paths. If we do the reparameterization
in createplan.c, we would be able to avoid all the translations.

I agree. The costs do not change because of reparameterization. The process
of creating paths is to estimate costs of different plans. So I think it
makes sense to delay anything which doesn't contribute to costing till the
final plan is decided.

However, if reparameterization can *not* happen at the planning time, we
have chosen a plan which can not be realised meeting a dead end. So as long
as we can ensure that the reparameterization is possible we can delay
actual translations. I think it should be possible to decide whether
reparameterization is possible based on the type of path alone. So seems
doable.

--
Best Wishes,
Ashutosh Bapat

On Thu, Aug 3, 2023 at 7:20 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

However, if reparameterization can *not* happen at the planning time, we
have chosen a plan which can not be realised meeting a dead end. So as long
as we can ensure that the reparameterization is possible we can delay
actual translations. I think it should be possible to decide whether
reparameterization is possible based on the type of path alone. So seems
doable.

That has been done in v2 patch. See the new added function
path_is_reparameterizable_by_child().

Thanks
Richard

On Fri, Aug 4, 2023 at 6:08 AM Richard Guo <guofenglinux@gmail.com> wrote:

On Thu, Aug 3, 2023 at 7:20 PM Ashutosh Bapat <
ashutosh.bapat.oss@gmail.com> wrote:

However, if reparameterization can *not* happen at the planning time, we
have chosen a plan which can not be realised meeting a dead end. So as long
as we can ensure that the reparameterization is possible we can delay
actual translations. I think it should be possible to decide whether
reparameterization is possible based on the type of path alone. So seems
doable.

That has been done in v2 patch. See the new added function
path_is_reparameterizable_by_child().

Thanks. The patch looks good overall. I like it because of its potential to
reduce memory consumption in reparameterization. That's cake on top of
cream :)

Some comments here.

+ {
+ FLAT_COPY_PATH(new_path, path, Path);
+
+ if (path->pathtype == T_SampleScan)
+ {
+ Index scan_relid = path->parent->relid;
+ RangeTblEntry *rte;
+
+ /* it should be a base rel with a tablesample clause... */
+ Assert(scan_relid > 0);
+ rte = planner_rt_fetch(scan_relid, root);
+ Assert(rte->rtekind == RTE_RELATION);
+ Assert(rte->tablesample != NULL);
+
+ ADJUST_CHILD_EXPRS(rte->tablesample, TableSampleClause *);
+ }
+ }

This change makes this function usable only after the final plan has been
selected. If some code accidently uses it earlier, it would corrupt
rte->tablesample without getting detected easily. I think we should mention
this in the function prologue and move it somewhere else. Other option is
to a.
leave rte->tablesample unchanged here with a comment as to why we aren't
changing it b. reparameterize tablesample expression in
create_samplescan_plan() if the path is parameterized by the parent. We call
reparameterize_path_by_child() in create_nestloop_plan() as this patch does
right now. I like that we are delaying reparameterization. It saves a bunch
of
memory. I haven't measured it but from my recent experiments I know it will
be
a lot.

  * If the inner path is parameterized, it is parameterized by the
- * topmost parent of the outer rel, not the outer rel itself.  Fix
- * that.
+ * topmost parent of the outer rel, not the outer rel itself.  We will

There's something wrong with the original sentence (which was probably
written
by me back then :)). I think it should have read "If the inner path is
parameterized by the topmost parent of the outer rel instead of the outer
rel
itself, fix that.". If you agree, the new comment should change it
accordingly.

@@ -2430,6 +2430,16 @@ create_nestloop_path(PlannerInfo *root,
 {
  NestPath   *pathnode = makeNode(NestPath);
  Relids inner_req_outer = PATH_REQ_OUTER(inner_path);
+ Relids outerrelids;
+
+ /*
+ * Paths are parameterized by top-level parents, so run parameterization
+ * tests on the parent relids.
+ */
+ if (outer_path->parent->top_parent_relids)
+ outerrelids = outer_path->parent->top_parent_relids;
+ else
+ outerrelids = outer_path->parent->relids;

This looks like an existing bug. Did partitionwise join paths ended up with
extra restrict clauses without this fix? I am not able to see why this
change
is needed by rest of the changes in the patch?

Anyway, let's rename outerrelids to something else e.g. outer_param_relids
to reflect
its true meaning.

+bool
+path_is_reparameterizable_by_child(Path *path)
+{
+ switch (nodeTag(path))
+ {
+ case T_Path:
... snip ...
+ case T_GatherPath:
+ return true;
+ default:
+
+ /* We don't know how to reparameterize this path. */
+ return false;
+ }
+
+ return false;
+}

How do we make sure that any changes to reparameterize_path_by_child() are
reflected here? One way is to call this function from
reparameterize_path_by_child() the first thing and return if the path can
not
be reparameterized.

I haven't gone through each path's translation to make sure that it's
possible
to do that during create_nestloop_plan(). I will do that in my next round of
review if time permits. But AFAIR, each of the translations are possible
during
create_nestloop_plan() when it happens in this patch.

-#define ADJUST_CHILD_ATTRS(node) \
+#define ADJUST_CHILD_EXPRS(node, fieldtype) \
  ((node) = \
- (List *) adjust_appendrel_attrs_multilevel(root, (Node *) (node), \
- child_rel, \
- child_rel->top_parent))
+ (fieldtype) adjust_appendrel_attrs_multilevel(root, (Node *) (node), \
+   child_rel, \
+   child_rel->top_parent))
+
+#define ADJUST_CHILD_ATTRS(node) ADJUST_CHILD_EXPRS(node, List *)

IIRC, ATTRS here is taken from the function being called. I think we should
just change the macro definition, not its name. If you would like to have a
separate macro for List nodes, create ADJUST_CHILD_ATTRS_IN_LIST or some
such.

--
Best Wishes,
Ashutosh Bapat

On Mon, Aug 7, 2023 at 9:29 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

Thanks. The patch looks good overall. I like it because of its potential
to reduce memory consumption in reparameterization. That's cake on top of
cream :)

Thanks for reviewing this patch!

Some comments here.

+ {
+ FLAT_COPY_PATH(new_path, path, Path);
+
+ if (path->pathtype == T_SampleScan)
+ {
+ Index scan_relid = path->parent->relid;
+ RangeTblEntry *rte;
+
+ /* it should be a base rel with a tablesample clause... */
+ Assert(scan_relid > 0);
+ rte = planner_rt_fetch(scan_relid, root);
+ Assert(rte->rtekind == RTE_RELATION);
+ Assert(rte->tablesample != NULL);
+
+ ADJUST_CHILD_EXPRS(rte->tablesample, TableSampleClause *);
+ }
+ }

This change makes this function usable only after the final plan has been
selected. If some code accidently uses it earlier, it would corrupt
rte->tablesample without getting detected easily. I think we should mention
this in the function prologue and move it somewhere else. Other option is
to a.
leave rte->tablesample unchanged here with a comment as to why we aren't
changing it b. reparameterize tablesample expression in
create_samplescan_plan() if the path is parameterized by the parent. We
call
reparameterize_path_by_child() in create_nestloop_plan() as this patch does
right now. I like that we are delaying reparameterization. It saves a
bunch of
memory. I haven't measured it but from my recent experiments I know it
will be
a lot.

I agree that we should mention in the function's comment that
reparameterize_path_by_child can only be used after the best path has
been selected because the RTE might be modified by this function. I'm
not sure if it's a good idea to move the reparameterization of
tablesample to create_samplescan_plan(). That would make the
reparameterization work separated in different places. And in the
future we may find more cases where we need modify RTEs or RELs for
reparameterization. It seems better to me that we keep all the work in
one place.

* If the inner path is parameterized, it is parameterized by the
- * topmost parent of the outer rel, not the outer rel itself.  Fix
- * that.
+ * topmost parent of the outer rel, not the outer rel itself.  We will

There's something wrong with the original sentence (which was probably
written
by me back then :)). I think it should have read "If the inner path is
parameterized by the topmost parent of the outer rel instead of the outer
rel
itself, fix that.". If you agree, the new comment should change it
accordingly.

Right. Will do that.

@@ -2430,6 +2430,16 @@ create_nestloop_path(PlannerInfo *root,
{
NestPath   *pathnode = makeNode(NestPath);
Relids inner_req_outer = PATH_REQ_OUTER(inner_path);
+ Relids outerrelids;
+
+ /*
+ * Paths are parameterized by top-level parents, so run parameterization
+ * tests on the parent relids.
+ */
+ if (outer_path->parent->top_parent_relids)
+ outerrelids = outer_path->parent->top_parent_relids;
+ else
+ outerrelids = outer_path->parent->relids;

This looks like an existing bug. Did partitionwise join paths ended up with
extra restrict clauses without this fix? I am not able to see why this
change
is needed by rest of the changes in the patch?

This is not an existing bug. Our current code (without this patch)
would always call create_nestloop_path() after the reparameterization
work has been done for the inner path. So we can safely check against
outer rel (not its topmost parent) in create_nestloop_path(). But now
with this patch, the reparameterization is postponed until createplan.c,
so we have to check against the topmost parent of outer rel in
create_nestloop_path(), otherwise we'd have duplicate clauses in the
final plan. For instance, without this change you'd get a plan like

regression=# explain (costs off)
select * from prt1 t1 left join lateral
(select t1.a as t1a, t2.* from prt1 t2) s on t1.a = s.a;
QUERY PLAN
---------------------------------------------------------
Append
-> Nested Loop Left Join
Join Filter: (t1_1.a = t2_1.a)
-> Seq Scan on prt1_p1 t1_1
-> Index Scan using iprt1_p1_a on prt1_p1 t2_1
Index Cond: (a = t1_1.a)
-> Nested Loop Left Join
Join Filter: (t1_2.a = t2_2.a)
-> Seq Scan on prt1_p2 t1_2
-> Index Scan using iprt1_p2_a on prt1_p2 t2_2
Index Cond: (a = t1_2.a)
-> Nested Loop Left Join
Join Filter: (t1_3.a = t2_3.a)
-> Seq Scan on prt1_p3 t1_3
-> Index Scan using iprt1_p3_a on prt1_p3 t2_3
Index Cond: (a = t1_3.a)
(16 rows)

Note that the clauses in Join Filter are duplicate.

Anyway, let's rename outerrelids to something else e.g.
outer_param_relids to reflect
its true meaning.

Hmm, I'm not sure this is a good idea. Here the 'outerrelids' is just
the relids of the outer rel or outer rel's topmost parent. I think it's
better to keep it as-is, and this is consistent with 'outerrelids' in
try_nestloop_path().

+bool
+path_is_reparameterizable_by_child(Path *path)
+{
+ switch (nodeTag(path))
+ {
+ case T_Path:
... snip ...
+ case T_GatherPath:
+ return true;
+ default:
+
+ /* We don't know how to reparameterize this path. */
+ return false;
+ }
+
+ return false;
+}

How do we make sure that any changes to reparameterize_path_by_child() are
reflected here? One way is to call this function from
reparameterize_path_by_child() the first thing and return if the path can
not
be reparameterized.

Good question. But I don't think calling this function at the beginning
of reparameterize_path_by_child() can solve this problem. Even if we do
that, if we find that the path cannot be reparameterized in
reparameterize_path_by_child(), it would be too late for us to take any
measures. So we need to make sure that such situation cannot happen. I
think we can emphasize this point in the comments of the two functions,
and meanwhile add an Assert in reparameterize_path_by_child() that the
path must be reparameterizable.

-#define ADJUST_CHILD_ATTRS(node) \
+#define ADJUST_CHILD_EXPRS(node, fieldtype) \
((node) = \
- (List *) adjust_appendrel_attrs_multilevel(root, (Node *) (node), \
- child_rel, \
- child_rel->top_parent))
+ (fieldtype) adjust_appendrel_attrs_multilevel(root, (Node *) (node), \
+   child_rel, \
+   child_rel->top_parent))
+
+#define ADJUST_CHILD_ATTRS(node) ADJUST_CHILD_EXPRS(node, List *)

IIRC, ATTRS here is taken from the function being called. I think we should
just change the macro definition, not its name. If you would like to have a
separate macro for List nodes, create ADJUST_CHILD_ATTRS_IN_LIST or some
such.

Agreed. I introduced the new macro ADJUST_CHILD_EXPRS just to keep
macro ADJUST_CHILD_ATTRS as-is, so callers to ADJUST_CHILD_ATTRS can
keep unchanged. It seems better to just change ADJUST_CHILD_ATTRS as
well as its callers. Will do that.

Attaching v3 patch to address all the reviews above.

Thanks
Richard

On Tue, Aug 8, 2023 at 12:44 PM Richard Guo <guofenglinux@gmail.com> wrote:

I agree that we should mention in the function's comment that
reparameterize_path_by_child can only be used after the best path has
been selected because the RTE might be modified by this function. I'm
not sure if it's a good idea to move the reparameterization of
tablesample to create_samplescan_plan(). That would make the
reparameterization work separated in different places. And in the
future we may find more cases where we need modify RTEs or RELs for
reparameterization. It seems better to me that we keep all the work in
one place.

Let's see what a committer thinks. Let's leave it like that for now.

This is not an existing bug. Our current code (without this patch)
would always call create_nestloop_path() after the reparameterization
work has been done for the inner path. So we can safely check against
outer rel (not its topmost parent) in create_nestloop_path(). But now
with this patch, the reparameterization is postponed until createplan.c,
so we have to check against the topmost parent of outer rel in
create_nestloop_path(), otherwise we'd have duplicate clauses in the
final plan.

Hmm. I am worried about the impact this will have when the nestloop
path is created for two child relations. Your code will still pick the
top_parent_relids which seems odd. Have you tested that case?

For instance, without this change you'd get a plan like

regression=# explain (costs off)
select * from prt1 t1 left join lateral
(select t1.a as t1a, t2.* from prt1 t2) s on t1.a = s.a;
QUERY PLAN

... snip ...

Note that the clauses in Join Filter are duplicate.

Thanks for the example.

Good question. But I don't think calling this function at the beginning
of reparameterize_path_by_child() can solve this problem. Even if we do
that, if we find that the path cannot be reparameterized in
reparameterize_path_by_child(), it would be too late for us to take any
measures. So we need to make sure that such situation cannot happen. I
think we can emphasize this point in the comments of the two functions,
and meanwhile add an Assert in reparameterize_path_by_child() that the
path must be reparameterizable.

Works for me.

Attaching v3 patch to address all the reviews above.

The patch looks good to me.

Please add this to the next commitfest.

--
Best Wishes,
Ashutosh Bapat

On Tue, Aug 8, 2023 at 7:34 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
wrote:

On Tue, Aug 8, 2023 at 12:44 PM Richard Guo <guofenglinux@gmail.com>
wrote:

This is not an existing bug. Our current code (without this patch)
would always call create_nestloop_path() after the reparameterization
work has been done for the inner path. So we can safely check against
outer rel (not its topmost parent) in create_nestloop_path(). But now
with this patch, the reparameterization is postponed until createplan.c,
so we have to check against the topmost parent of outer rel in
create_nestloop_path(), otherwise we'd have duplicate clauses in the
final plan.

Hmm. I am worried about the impact this will have when the nestloop
path is created for two child relations. Your code will still pick the
top_parent_relids which seems odd. Have you tested that case?

Well, the way it's working is that for child rels all parameterized
paths arriving at try_nestloop_path (or try_partial_nestloop_path) are
parameterized by top parents at first. Then our current code (without
this patch) applies reparameterize_path_by_child to the inner path
before calling create_nestloop_path(). So in create_nestloop_path() the
inner path is parameterized by child rel. That's why we check against
outer rel itself not its top parent there.

With this patch, the reparameterize_path_by_child work is postponed
until createplan time, so in create_nestloop_path() the inner path is
still parameterized by top parent. So we have to check against the top
parent of outer rel.

I think the query shown upthread is sufficient to verify this theory.

regression=# explain (costs off)
select * from prt1 t1 left join lateral
(select t1.a as t1a, t2.* from prt1 t2) s on t1.a = s.a;
QUERY PLAN
---------------------------------------------------------
Append
-> Nested Loop Left Join
-> Seq Scan on prt1_p1 t1_1
-> Index Scan using iprt1_p1_a on prt1_p1 t2_1
Index Cond: (a = t1_1.a)
-> Nested Loop Left Join
-> Seq Scan on prt1_p2 t1_2
-> Index Scan using iprt1_p2_a on prt1_p2 t2_2
Index Cond: (a = t1_2.a)
-> Nested Loop Left Join
-> Seq Scan on prt1_p3 t1_3
-> Index Scan using iprt1_p3_a on prt1_p3 t2_3
Index Cond: (a = t1_3.a)
(13 rows)

Please add this to the next commitfest.

Done here https://commitfest.postgresql.org/44/4477/

Thanks
Richard

On Wed, Aug 9, 2023 at 8:14 AM Richard Guo <guofenglinux@gmail.com> wrote:

With this patch, the reparameterize_path_by_child work is postponed
until createplan time, so in create_nestloop_path() the inner path is
still parameterized by top parent. So we have to check against the top
parent of outer rel.

Your changes in create_nestloop_path() only affect the check for
parameterized path not the unparameterized paths. So I think we are
good there. My worry was misplaced.

--
Best Wishes,
Ashutosh Bapat

I looked over the v3 patch. I think it's going in generally
the right direction, but there is a huge oversight:
path_is_reparameterizable_by_child does not come close to modeling
the success/failure conditions of reparameterize_path_by_child.
In all the cases where reparameterize_path_by_child can recurse,
path_is_reparameterizable_by_child has to do so also, to check
whether the child path is reparameterizable. (I'm somewhat
disturbed that apparently we have no test cases that caught that
oversight; can we add one cheaply?)

BTW, I also note from the cfbot that 9e9931d2b broke this patch by
adding more ADJUST_CHILD_ATTRS calls that need to be modified.
I wonder if we could get away with having that macro cast to "void *"
to avoid needing to change all its call sites. I'm not sure whether
pickier compilers might warn about doing it that way.

regards, tom lane

On Sun, Aug 20, 2023 at 11:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I looked over the v3 patch. I think it's going in generally
the right direction, but there is a huge oversight:
path_is_reparameterizable_by_child does not come close to modeling
the success/failure conditions of reparameterize_path_by_child.
In all the cases where reparameterize_path_by_child can recurse,
path_is_reparameterizable_by_child has to do so also, to check
whether the child path is reparameterizable. (I'm somewhat
disturbed that apparently we have no test cases that caught that
oversight; can we add one cheaply?)

Thanks for pointing this out. It's my oversight. We have to check the
child path(s) recursively to tell if a path is reparameterizable or not.
I've fixed this in v4 patch, along with a test case. In the test case
we have a MemoizePath whose subpath is TidPath, which is not
reparameterizable. This test case would trigger Assert in v3 patch.

BTW, I also note from the cfbot that 9e9931d2b broke this patch by
adding more ADJUST_CHILD_ATTRS calls that need to be modified.
I wonder if we could get away with having that macro cast to "void *"
to avoid needing to change all its call sites. I'm not sure whether
pickier compilers might warn about doing it that way.

Agreed and have done that in v4 patch.

Thanks
Richard

On Mon, Aug 21, 2023 at 4:09 PM Richard Guo <guofenglinux@gmail.com> wrote:

On Sun, Aug 20, 2023 at 11:48 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I looked over the v3 patch. I think it's going in generally
the right direction, but there is a huge oversight:
path_is_reparameterizable_by_child does not come close to modeling
the success/failure conditions of reparameterize_path_by_child.
In all the cases where reparameterize_path_by_child can recurse,
path_is_reparameterizable_by_child has to do so also, to check
whether the child path is reparameterizable. (I'm somewhat
disturbed that apparently we have no test cases that caught that
oversight; can we add one cheaply?)

Thanks for pointing this out. It's my oversight. We have to check the
child path(s) recursively to tell if a path is reparameterizable or not.
I've fixed this in v4 patch, along with a test case. In the test case
we have a MemoizePath whose subpath is TidPath, which is not
reparameterizable. This test case would trigger Assert in v3 patch.

This goes back to my question about how do we keep
path_is_reparameterizable_by_child() and
reparameterize_path_by_child() in sync with each other. This makes it
further hard to do so. One idea I have is to use the same function
(reparameterize_path_by_child()) in two modes 1. actual
reparameterization 2. assessing whether reparameterization is
possible. Essentially combing reparameterize_path_by_child() and
path_is_reparameterizable_by_child(). The name of such a function can
be chosen appropriately. The mode will be controlled by a fourth
argument to the function. When assessing a path no translations happen
and no extra memory is allocated.

--
Best Wishes,
Ashutosh Bapat

I spent some time reviewing the v4 patch. I noted that
path_is_reparameterizable_by_child still wasn't modeling the pass/fail
behavior of reparameterize_path_by_child very well, because that
function checks this at every recursion level:

/*
* If the path is not parameterized by the parent of the given relation,
* it doesn't need reparameterization.
*/
if (!path->param_info ||
!bms_overlap(PATH_REQ_OUTER(path), child_rel->top_parent_relids))
return path;

So we might have a sub-path (eg a join input rel) that is not one of
the supported kinds, and yet we can succeed because parameterization
appears only in other sub-paths. The patch as written will not crash
in such a case, but it might refuse to use a path that we previously
would have allowed. So I think we need to put the same test into
path_is_reparameterizable_by_child, which requires adding child_rel
to its param list but is otherwise simple enough.

I also realized that this test is equivalent to
PATH_PARAM_BY_PARENT(), which makes it really unnecessary for
createplan.c to test PATH_PARAM_BY_PARENT, so we don't need to
expose those macros globally after all.

(On the same logic, we could skip PATH_PARAM_BY_PARENT at the
call sites of path_is_reparameterizable_by_child. I didn't
do that in the attached, mainly because it seems to make it
harder to understand/explain what is being tested.)

Another change I made is to put the path_is_reparameterizable_by_child
tests before the initial_cost_nestloop/add_path_precheck steps, on
the grounds that they're now cheap enough that we might as well do
them first. The existing ordering of these steps was sensible when
we were doing the expensive reparameterization, but it seems a bit
unnatural IMO.

Lastly, although I'd asked for a test case demonstrating detection of
an unparameterizable sub-path, I ended up not using that, because
it seemed pretty fragile. If somebody decides that
reparameterize_path_by_child ought to cover TidPaths, the test won't
prove anything any more.

So that led me to the attached v5, which seemed committable to me so I
set about back-patching it ... and it fell over immediately in v15, as
shown in the attached regression diffs from v15. It looks to me like
we are now failing to recognize that reparameterized quals are
redundant with not-reparameterized ones, so this patch is somehow
dependent on restructuring that happened during the v16 cycle.
I don't have time to dig deeper than that, and I'm not sure that that
is an area we'd want to mess with in a back-patched bug fix.

What I'm thinking we ought to do instead for the back branches
is just refuse to generate a reparameterized path for tablesample
scans. A minimal fix like that could be as little as

        case T_Path:
+           if (path->pathtype == T_SampleScan)
+               return NULL;
            FLAT_COPY_PATH(new_path, path, Path);
            break;

This rejects more than it absolutely has to, because the
parameterization (that we know exists) might be in the path's
regular quals or tlist rather than in the tablesample.
So we could add something to see if the tablesample is
parameter-free, but I'm quite unsure that it's worth the
trouble. There must be just about nobody using such cases,
or we'd have heard of this bug long ago.

(BTW, I did look at Ashutosh's idea of merging the
reparameterize_path_by_child and path_is_reparameterizable_by_child
functions, but I didn't think that would be an improvement,
because we'd have to clutter reparameterize_path_by_child with
a lot of should-I-skip-this-step tests. Some of that could be
hidden in the macros, but a lot would not be. Another issue
is that I do not think we can change reparameterize_path_by_child's
API contract in the back branches, because we advertise it as
something that FDWs and custom scan providers can use.)

Thoughts?

regards, tom lane

On Tue, Aug 22, 2023 at 4:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

I spent some time reviewing the v4 patch. I noted that
path_is_reparameterizable_by_child still wasn't modeling the pass/fail
behavior of reparameterize_path_by_child very well, because that
function checks this at every recursion level:

/*
* If the path is not parameterized by the parent of the given
relation,
* it doesn't need reparameterization.
*/
if (!path->param_info ||
!bms_overlap(PATH_REQ_OUTER(path), child_rel->top_parent_relids))
return path;

So we might have a sub-path (eg a join input rel) that is not one of
the supported kinds, and yet we can succeed because parameterization
appears only in other sub-paths. The patch as written will not crash
in such a case, but it might refuse to use a path that we previously
would have allowed. So I think we need to put the same test into
path_is_reparameterizable_by_child, which requires adding child_rel
to its param list but is otherwise simple enough.

sigh.. I overlooked this check. You're right. We have to have this
same test in path_is_reparameterizable_by_child.

So that led me to the attached v5, which seemed committable to me so I
set about back-patching it ... and it fell over immediately in v15, as
shown in the attached regression diffs from v15. It looks to me like
we are now failing to recognize that reparameterized quals are
redundant with not-reparameterized ones, so this patch is somehow
dependent on restructuring that happened during the v16 cycle.
I don't have time to dig deeper than that, and I'm not sure that that
is an area we'd want to mess with in a back-patched bug fix.

I looked at this and I think the culprit is that in create_nestloop_path
we are failing to recognize those restrict_clauses that have been moved
into the inner path. In v16, we have the new serial number stuff to
help detect such clauses and that works very well. In v15, we are still
using join_clause_is_movable_into() for that purpose. It does not work
well with the patch because now in create_nestloop_path the inner path
is still parameterized by top-level parents, while the restrict_clauses
already have been adjusted to refer to the child rels. So the check
performed by join_clause_is_movable_into() would always fail.

I'm wondering if we can instead adjust the 'inner_req_outer' in
create_nestloop_path before we perform the check to work around this
issue for the back branches, something like

@@ -2418,6 +2419,15 @@ create_nestloop_path(PlannerInfo *root,
NestPath *pathnode = makeNode(NestPath);
Relids inner_req_outer = PATH_REQ_OUTER(inner_path);

+   /*
+    * Adjust the parameterization information, which refers to the topmost
+    * parent.
+    */
+   inner_req_outer =
+       adjust_child_relids_multilevel(root, inner_req_outer,
+                                      outer_path->parent->relids,
+
 outer_path->parent->top_parent_relids);
+

And with it we do not need the changes as in the patch for
create_nestloop_path any more.

Thanks
Richard

Richard Guo <guofenglinux@gmail.com> writes:

On Tue, Aug 22, 2023 at 4:48 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

So that led me to the attached v5, which seemed committable to me so I
set about back-patching it ... and it fell over immediately in v15, as
shown in the attached regression diffs from v15. It looks to me like
we are now failing to recognize that reparameterized quals are
redundant with not-reparameterized ones, so this patch is somehow
dependent on restructuring that happened during the v16 cycle.
I don't have time to dig deeper than that, and I'm not sure that that
is an area we'd want to mess with in a back-patched bug fix.

I looked at this and I think the culprit is that in create_nestloop_path
we are failing to recognize those restrict_clauses that have been moved
into the inner path. In v16, we have the new serial number stuff to
help detect such clauses and that works very well. In v15, we are still
using join_clause_is_movable_into() for that purpose. It does not work
well with the patch because now in create_nestloop_path the inner path
is still parameterized by top-level parents, while the restrict_clauses
already have been adjusted to refer to the child rels. So the check
performed by join_clause_is_movable_into() would always fail.

Ah.

I'm wondering if we can instead adjust the 'inner_req_outer' in
create_nestloop_path before we perform the check to work around this
issue for the back branches, something like
+   /*
+    * Adjust the parameterization information, which refers to the topmost
+    * parent.
+    */
+   inner_req_outer =
+       adjust_child_relids_multilevel(root, inner_req_outer,
+                                      outer_path->parent->relids,
+                                      outer_path->parent->top_parent_relids);

Mmm ... at the very least you'd need to not do that when top_parent_relids
is unset. But I think the risk/reward ratio for messing with this in the
back branches is unattractive in any case: to fix a corner case that
apparently nobody uses in the field, we risk breaking any number of
mainstream parameterized-path cases. I'm content to commit the v5 patch
(or a successor) into HEAD, but at this point I'm not sure I even want
to risk it in v16, let alone perform delicate surgery to get it to work
in older branches. I think we ought to go with the "tablesample scans
can't be reparameterized" approach in v16 and before.

regards, tom lane

I wrote:

... I think the risk/reward ratio for messing with this in the
back branches is unattractive in any case: to fix a corner case that
apparently nobody uses in the field, we risk breaking any number of
mainstream parameterized-path cases. I'm content to commit the v5 patch
(or a successor) into HEAD, but at this point I'm not sure I even want
to risk it in v16, let alone perform delicate surgery to get it to work
in older branches. I think we ought to go with the "tablesample scans
can't be reparameterized" approach in v16 and before.

Concretely, about like this for v16, and similarly in older branches.

regards, tom lane

Hi Tom,

On Tue, Aug 22, 2023 at 2:18 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:

(BTW, I did look at Ashutosh's idea of merging the
reparameterize_path_by_child and path_is_reparameterizable_by_child
functions, but I didn't think that would be an improvement,
because we'd have to clutter reparameterize_path_by_child with
a lot of should-I-skip-this-step tests. Some of that could be
hidden in the macros, but a lot would not be. Another issue
is that I do not think we can change reparameterize_path_by_child's
API contract in the back branches, because we advertise it as
something that FDWs and custom scan providers can use.)

Here are two patches
0001 same as your v5 patch
0002 implements what I had in mind - combining the assessment and
reparameterization in a single function.

I don't know whether you had something similar to 0002 when you
considered that approach. Hence implemented it quickly. The names of
the functions and arguments can be changed. But I think this version
will help us keep assessment and actual reparameterization in sync,
very tightly. Most of the conditionals have been pushed into macros,
except for two which need to be outside the macros and actually look
better to me. Let me know what you think of this.

FWIW I ran make check and all the tests pass.

I agree that we can not consider this for backbranches but we are
anyway thinking of disabling reparameterization for tablesample scans
anyway.

--
Best Wishes,
Ashutosh Bapat