Re: Oversight in reparameterize_path_by_child leading to executor crash

On 6/12/2023 14:30, Richard Guo wrote:
> I've self-reviewed this patch again and I think it's now in a
> committable state.  I'm wondering if we can mark it as 'Ready for
> Committer' now, or we need more review comments/feedbacks.
>
> To recap, this patch postpones reparameterization of paths until
> createplan.c, which would help avoid building the reparameterized
> expressions we might not use.  More importantly, it makes it possible to
> modify the expressions in RTEs (e.g. tablesample) and in RelOptInfos
> (e.g. baserestrictinfo) for reparameterization.  Failing to do that can
> lead to executor crashes, wrong results, or planning errors, as we have
> already seen.

As I see, this patch contains the following modifications:
1. Changes in the create_nestloop_path: here, it arranges all tests to
the value of top_parent_relids, if any. It is ok for me.
2. Changes in reparameterize_path_by_child and coupled new function
path_is_reparameterizable_by_child. I compared them, and it looks good.
One thing here. You use the assertion trap in the case of inconsistency
in the behaviour of these two functions. How disastrous would it be if
someone found such inconsistency in production? Maybe better to use
elog(PANIC, ...) report?
3. ADJUST_CHILD_ATTRS() macros. Understanding the necessity of this
change, I want to say it looks a bit ugly.
4. SampleScan reparameterization part. It looks ok. It can help us in
the future with asymmetric join and something else.
5. Tests. All of them are related to partitioning and the SampleScan
issue. Maybe it is enough, but remember, this change now impacts the
parameterization feature in general.
6. I can't judge how this switch of overall approach could impact
something in the planner. I am only wary about what, from the first
reparameterization up to the plan creation, we have some inconsistency
in expressions (partitions refer to expressions with the parent relid).
What if an extension in the middle changes the parent expression?

By and large, this patch is in a good state and may be committed.

--
regards,
Andrei Lepikhov
Postgres Professional