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>
 <CA+TgmobzR+XMGbRosVPbjHbSo4+cgJn=qZK6w05aF1sbj=C+9Q@mail.gmail.com>
 <CA+TgmoawzvCoZAwFS85tE5+c8vBkqgcS8ZstQ_ohjXQ9wGT9sw@mail.gmail.com>
 <CA+TgmoYS4ZCVAF2jTce=bMP0Oq_db_srocR4cZyO0OBp9oUoGg@mail.gmail.com>
 <CAK98qZ2RzbgCHrSg4zLkvpzyBam_X6te-KF8w1+_vON9BAVMEw@mail.gmail.com>
 <CA+TgmoaCdsuvNn6T6SfQ_0YD2Hh2+hgTXh9fTGHQhPg1zvy2rQ@mail.gmail.com>
 <CAK98qZ1zWzRB0ABG7ULzTeWKRR5C7-FxLqM-6v8wQDiFM+DNAg@mail.gmail.com>
 <CA+Tgmob7ozJAs5SU6bD2RfAt4w_AmsMGz-aaVA6WeLXHkBypOg@mail.gmail.com>
 <CAK98qZ1J42RoAsHnYWGPPmHziZmzmqE=Lp_O6WJ-9aKK2qjikA@mail.gmail.com>
 <CA+TgmoYjcBA6dw3nwiyfDzPXTCrxTZPXDMrc2TrDJcL1cPK6iA@mail.gmail.com>
 <CA+TgmoYru-vxoTKfwjQby30r2OkTXfb18Km_=VLs6qk8Akr0-g@mail.gmail.com>
 <CAKFQuwYxTMx1e1dvVLd5vs8CO2Xe__YRCW0m4v2UErG=5Lu7kQ@mail.gmail.com>
 <CA+TgmoaPH3rOmH1zw8s10Az3TQKRbDnF3dKYZCAFvx_DgFV5_Q@mail.gmail.com>
 <CAKFQuwZ7CRbvmJ60EjvEUJP8X9hWVCHJz8yJZ0-xRpYwL=fugA@mail.gmail.com>
 <CA+TgmoYsG2pAOG4YYWwnGsaL+QfOZopi_m+f_hZqBZ3meNEkjg@mail.gmail.com>
 <CAKFQuwY4i6xXCRHd=ccrFz0sQOfiWRxhobiBrzyK0rzu1-Y33Q@mail.gmail.com>
 <CA+Tgmobbj_TaCsYmr1grJBTRKaFaxFfotXn1T6LSXs9GQ8_Kyw@mail.gmail.com>
 <CA+TgmoZzM2i+p-Rxdphs4qx7sshn-kzxF91ASQ5duOo0dFRXLQ@mail.gmail.com>
 <CA+Tgmoau7yJtvbeH-0kPt1Q=Gt_ezRdgM35Q1=LT665U_86Etg@mail.gmail.com>
 <CA+TgmoYRJdDh7MPK-cLs4w4gmYqEaWM3Y0o+2-YAKfJ=5rHNyA@mail.gmail.com>
In-Reply-To: 
 <CA+TgmoYRJdDh7MPK-cLs4w4gmYqEaWM3Y0o+2-YAKfJ=5rHNyA@mail.gmail.com>
From: Lukas Fittl <lukas@fittl.com>
Date: Wed, 18 Mar 2026 11:59:46 -0700
Message-ID: 
 <CAP53PkxpPsB+xtuC1zKt=6ANw8vHjX_SJe2k=0AguMAqaK67Lg@mail.gmail.com>
Subject: Re: pg_plan_advice
To: Robert Haas <robertmhaas@gmail.com>
Cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
 Tom Lane <tgl@sss.pgh.pa.us>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: 
 <https://www.postgresql.org/message-id/CAP53PkxpPsB%2BxtuC1zKt%3D6ANw8vHjX_SJe2k%3D0AguMAqaK67Lg%40mail.gmail.com>
Precedence: bulk

On Wed, Mar 18, 2026 at 10:08=E2=80=AFAM Robert Haas <robertmhaas@gmail.com=
> wrote:
> But this does raise two points which are perhaps worthy of some
> further consideration:
>
> 1. Maybe we should limit the length of the detail message. In some
> other cases, like reportDependentObjects, we limit the detail message
> to the first 100 problems found, and then say at the end of that
> detail message how many more problems there were afterwards. We could
> do that here, too. I'm not 100% sure it's worth the code, but then
> again it's not much code.

I think we have similar problems elsewhere in Postgres where a large
user input causes an even larger log output - e.g. a case I'm familiar
with are complicated queries with long IN list inputs and their
associated EXPLAIN plans being logged by auto_explain - I recently had
a case where someone reported an OOM due to auto_explain trying to log
a > 100 MB sized query plan.

I think its actually less a problem with plan advice, since presumably
we won't have ORMs generating plan advice, and even if we do it'd be
per-table - so I think its unlikely a genuine use case would use 1000s
of advice tags.

That said, I also don't think super long long messages are actually
helpful. I do wonder if we should have a more coarse GUC that limits
DETAIL lines of any kind to a maximum length (e.g. 100 kB) across the
board instead of special casing every emitter.

> 2. The way the current code works is to transform the advice feedback
> into a Node-tree representation which is stashed in the PlannedStmt's
> extension_state, and then also passes that Node-tree representation to
> pgpa_planner_feedback_warning, which generates the actual warning.
> That Node-tree representation is currently used by EXPLAIN to display
> the advice feedback, which I think for most people will be a nicer
> interface than enabling pg_plan_advice.feedback_warnings, and it can
> also be used by other extensions. For instance, we could extend
> pg_stash_advice so that it looks at the advice feedback and updates
> the shared store, so that users can monitor whether their stashed
> advice is doing what they hoped it would.

Yeah, I think the ability for other extensions to retrieve this is
pretty important - whether in pg_stash_advice, or any other kind of
plan management extension that wants to know the outcome of applied
advice.

>
> However, in a case like this, that Node tree is actually quite large:
> about 16MB. I guess that's because pgpa_planner_append_feedback() has
> to do multiple allocations for every item of feedback: a C string,
> DefElem, an Integer, plus whatever lappend() charges us to add to a
> List. We could save that memory by adding code here to optimize for
> the case where we need to generate warnings but we don't need the Node
> tree for anything else. I'm inclined not to do that, because (A) I
> don't think temporarily using 16MB when the user specifies 10,000
> items of bogus advice is really that bad and (B) complicating the code
> has its own costs, such as maybe introducing more bugs. However, maybe
> someone else sees it differently.
>
> Another idea is to try to find a more economic Node representation.
> For instance, we could jam the flags into the DefElem's location
> field, instead of building a separate Integer node, or we could build
> the advice feedback as a giant binary blob and wrap it in a varlena
> and a Const node and leave consumers to make sense of that as best
> they're able, or we could invent a new Node type that's just to the
> perfect thing to hold a C string and an integer. I'm inclined to think
> that the first two are too hacky and the third is too special-purpose,
> but again someone else might see it otherwise.

Yeah, I feel like we're definitely constrained here by the fact that
advice tags are defined by a contrib module vs in-core - if they were
in-core we could just add a dedicated node type for them. I don't
think inventing a specialized binary format only defined in the
contrib module makes sense.

Two other ideas:

1) What if we return the utilized advice string as a separate DefElem
with a list of strings, and then the feedback just has to reference an
index into that list? (though I suppose that doesn't actually save
memory, now that I think that through -- unless we assume the caller
already has the advice string, but I don't think we can rely on that)

2) We could consider having separate DefElems for the different flag
types (i.e. "feedback_failed", "feedback_match_full", etc), and then a
list of strings attached to each - that'd save the nested DefElem and
the Integer node

Thanks,
Lukas

--=20
Lukas Fittl