MIME-Version: 1.0
References: <CA+TgmoZ-Jh1T6QyWoCODMVQdhTUPYkaZjWztzP1En4=ZHoKPzw@mail.gmail.com>
 <CAKZiRmxtJAFG7e1+Vs9B8ngON=AOzJbuws+1ZeH4LsbJh5AzoQ@mail.gmail.com>
 <CA+TgmoY9Ne_Sh10u6LSPc3wvOQPLp3kF9nBp3nqJEG2JGF2QiA@mail.gmail.com>
 <CA+Tgmoa57S6mP=aTOXH2-gDAL4TMO1WbGgrHSg0s6J4zUH=04g@mail.gmail.com>
 <DEB1SAGVDX7S.JMV96X4R0RCS@gmail.com> <CA+Tgmoaf__2B0BUL+vrg28P+3buX=Ti-kybqkHiLTtFrrCfzuA@mail.gmail.com>
 <CA+TgmoYpcLNOuypOTdgCSLW7FuA=t6BtB3meTARHX2-Dj_81xQ@mail.gmail.com>
 <DEFNP3WX7BLX.1X7DS1P9X6JIF@nmfay.com> <CA+TgmoZjv9OyFu1Gkt78w0vWEti8S33w8trYHmErf-GMmGSi=w@mail.gmail.com>
 <DELPC4B56Y5A.3OBZ3WCH3T4VX@nmfay.com> <CA+TgmoaOSBQD9Ux4eG40w723ZN=c0J7p-+oX4+J8urUeyLMo5w@mail.gmail.com>
 <CAOYmi+=g+MMoOpWkk2weXWKJcKH0eKey8gKHHdH0dF4Tiawrhw@mail.gmail.com>
 <CA+TgmobwaT=PXPDDrgDup+jA8KHBbkxigtziD-zNzAKKkQYVgQ@mail.gmail.com>
 <CAOYmi+mOmEW=amDRQMfw6-Fb3ZmDEQFaJzwk8Bc8W8DzaP85XQ@mail.gmail.com>
 <CA+TgmoaX2AMW4cdFM3OngBJxmxpkdmzF33R7-CWhvRLfucbFMg@mail.gmail.com>
 <CAOYmi+k4AyWCQHK=XVF99KVDuFkqxcADao61OWGLxu0nRYMONQ@mail.gmail.com>
 <CA+TgmoZ0x3ym_oueXRWzbM_=6ucKoPZVGj3rRMLBDC_FnetXDw@mail.gmail.com>
 <CAP53Pkycc=7N2bLzVT3x+qE1JamvRZWev5tFjdLJ1+-AV3Di+Q@mail.gmail.com>
 <CA+TgmoaKhuD91RnazbRyGkmP7--JdNq8oNDC3UcgTZSWbMxC7w@mail.gmail.com>
 <CAP53Pkw5-wMEeDJXFmqo_RTyL_spzCXb7HHKrbSnQqokVoZcNQ@mail.gmail.com>
 <CA+Tgmob-69bzbdi3U_QtebqAf6u1y8js=5=oNK639csVe1VbhA@mail.gmail.com>
 <CA+TgmoaZMOikxK=LqS+Jn+835h9S139JLGk-3LyETVXw5W5j=w@mail.gmail.com>
 <1136161.1769654478@sss.pgh.pa.us> <CAP53PkwZ1ZTMARKg6iEfAw9qzBhkjBitj-9gr_Jvy7k2AwGgWA@mail.gmail.com>
 <CAMbWs4--NuEUFE_xTo991TRXaZryE29jarJPDnVxoaQOYdt7tA@mail.gmail.com>
In-Reply-To: <CAMbWs4--NuEUFE_xTo991TRXaZryE29jarJPDnVxoaQOYdt7tA@mail.gmail.com>
From: Robert Haas <robertmhaas@gmail.com>
Date: Thu, 29 Jan 2026 09:10:11 -0500
Message-ID: <CA+TgmobzR+XMGbRosVPbjHbSo4+cgJn=qZK6w05aF1sbj=C+9Q@mail.gmail.com>
Subject: Re: pg_plan_advice
To: Richard Guo <guofenglinux@gmail.com>
Cc: Lukas Fittl <lukas@fittl.com>, Tom Lane <tgl@sss.pgh.pa.us>, 
	Jacob Champion <jacob.champion@enterprisedb.com>, Dian Fay <di@nmfay.com>, 
	Matheus Alcantara <matheusssilv97@gmail.com>, Jakub Wartak <jakub.wartak@enterprisedb.com>, 
	PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CA%2BTgmobzR%2BXMGbRosVPbjHbSo4%2BcgJn%3DqZK6w05aF1sbj%3DC%2B9Q%40mail.gmail.com>
Precedence: bulk

On Thu, Jan 29, 2026 at 1:44=E2=80=AFAM Richard Guo <guofenglinux@gmail.com=
> wrote:
> I think we should access spath->disabled_nodes until after we have
> verified that spath is not NULL.

Good catch. I have included a fix for this in
4020b370f214315b8c10430301898ac21658143f.

> Also, it's not quite clear to me why create_material_path (and
> cost_material) requires the "enabled" parameter.  I couldn't find any
> comments on this, so it might be worth adding some comments here.

I think cost_material() got an enabled argument because of
materialize_finished_plan(). Most of the time, plan nodes are created
from paths, and we want to use the parent's pgs_mask to determine
whether the chosen strategy (e.g. materialization) is enabled.
However, materialize_finished_plan() creates a plan directly, so
there's no RelOptInfo or JoinPathExtraData whose pgs_mask we can
consult, and so we have to fall back on consulting enable_material
directly. Once that parameter got added to cost_material(), it made
sense to me to add it to create_material_path() as well.

There are probably other ways I could have done this. For instance, we
could make create_material_path() pass true to cost_material(), and
not have its own "enabled" argument. That would mean that
reparameterize_path() would have to unconditionally pass true. I'm not
sure if that would be a problem. Another idea is to make
cost_material() consult enable_material itself if path->parent is
NULL, so that it wholly encapsulates the calculation locally.

One thing that is a little different about materialization vs. some
other operations is that it's used for multiple purposes. Sometimes we
use it because it's required for correctness, and other times we do it
to reduce cost. And, it can be used to affect cost in different
scenarios. In the Nested Loop case, PGS_NESTLOOP_MATERIALIZE should
control whether we are willing to materialize, but that doesn't apply
to other cases, like build_subplan() or create_mergejoin_plan().
Moreover, these latter cases have not yet been converted to use Paths
-- hopefully they will be converted someday, but maybe not soon. All
of this makes things a little more complicated for materialization vs.
something like a nested loop or a sequential scan, where the logic to
create that thing is all in one place and paths are in use.

But that is not to say that I'm deeply invested in cost_material()
having an "enabled" argument -- that seems like a detail to me that
could go either way. If it seems objectionable, we could change it,
and I think we could probably do so in more than one way and without
really affecting the behavior. Alternatively, we could leave it the
way it is and add a comment clarifying whatever portion of the above
you think is worth documenting in the source. It seemed like a
sufficiently minor detail to me that I did not bother, but that might
have been a mistake.

--=20
Robert Haas
EDB: http://www.enterprisedb.com