Horton

Re: Oversight in reparameterize_path_by_child leading to executor crash

Andrei Lepikhov <a.lepikhov@postgrespro.ru>

View thread
From: Andrei Lepikhov <a.lepikhov@postgrespro.ru>
To: Richard Guo <guofenglinux@gmail.com>, Alena Rybakina <lena.ribackina@yandex.ru>
Cc: Tom Lane <tgl@sss.pgh.pa.us>, Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>, PostgreSQL-development <pgsql-hackers@postgresql.org>
Date: 2023-12-13T02:55:02Z
Lists: pgsql-hackers

Commits

Same data as JSON: GET /api/v1/messages/:b64id/commits the thread's linked commits as JSON, with link sources. API reference →

  1. Postpone reparameterization of paths until create_plan().

  2. Apply band-aid fix for an oversight in reparameterize_path_by_child.

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