Re: ERROR: too late to create a new PlaceHolderInfo

From: Richard Guo <guofenglinux(at)gmail(dot)com>
To: Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us>
Cc: Pg Hackers <pgsql-hackers(at)lists(dot)postgresql(dot)org>
Subject: Re: ERROR: too late to create a new PlaceHolderInfo
Date: 2026-10-06 07:08:00
Message-ID: CAMbWs4_wukrbuyhoBp9qg===MdPvhZjkc5O7=HE0+UoQ-17NHg@mail.gmail.com
Views: Whole Thread | Raw Message | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

On Fri, Sep 18, 2026 at 12:41 AM Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> I wrote:
> > I agree with that sounding more principled, but I wonder if we should
> > think bigger than just tweaking add_nullingrels_if_needed: if we're
> > desirous of de-duplicating PHVs, why not do that across the board,
> > for every place that makes PHVs? So we'd mechanize this in
> > make_placeholder_expr's assignment of phid rather than somewhere else.

> Actually, we can't be too gung-ho about that: we should not merge
> PHVs if their expressions are volatile. It's not quite clear to me
> whether that's a problem for the join-alias-Vars case.

Thanks for looking at this.

I don't think volatility is a problem for the join alias case. A join
alias expansion starts out as Vars and COALESCEs of the join's inputs.
Those are handled without a PHV (is_standard_join_alias_expression).
We need a PHV only for a whole-row reference, which expands to a
RowExpr of those entries, or when an input Var has since been replaced
by some other expression. That happens through subquery or VALUES
pullup, which refuse volatile expressions.

As for doing this in make_placeholder_expr, I'm not sure it would buy
us much. pullup_replace_vars_callback already shares PHVs per
subquery output column through rv_cache. It doesn't have the problem
we have here, where the same reference exists in two copies of a
subquery that are expanded separately. Also, PHVs made during pullup
have their phrels rewritten afterwards, so cache entries for them
would go stale right away, and the lookups would mostly be wasted. So
I'm inclined to keep this in add_nullingrels_if_needed.

Regarding back-patching, I'm still hesitant. The new field is only
read in add_nullingrels_if_needed, so adding it at the end of the
struct would probably work. But if some extension does do
makeNode(PlannerInfo) and pass that to flatten_join_alias_vars, we'd
be reading a garbage pointer there. This patch might also lead to
plan changes, since expansions of the same join alias that used to get
separate PHVs now share one. But maybe we can get this patch into
v19.

- Richard

In response to

Browse pgsql-hackers by date

  From Date Subject
Previous Message Michael Paquier 2026-10-06 07:06:08 Re: Fix reindexdb with parallel index-level conrurrent run