Re: plan cache overhead on plpgsql expression - Mailing list pgsql-hackers

Andres Freund <andres@anarazel.de> writes:
> On 2020-03-25 17:51:50 -0400, Tom Lane wrote:
>> Perhaps, but I'm not sure that either of those functions represent
>> material overhead in cases besides this one.

> That's not huge, but also not nothing.

I see.  So maybe worth the trouble --- but still, seems like material for
a separate patch.

>>> Would it make sense to instead compute this as we go when building a
>>> valid CachedPlanSource?

>> I'm inclined to think not, because it'd just be overhead for other
>> users of cached plans.

> Even if we make RevalidateCachedQuery take advantage of the simpler
> tests when possible?

I'm not convinced that any real optimization is practical once you
allow tables in the query.  You then have to check the RLS-active
flags in some form, and the existing tests are not *that* expensive
as long as the answer is "no".  At best I think you might be reducing
two or three simple tests to one.

Also, the reason why this is interesting at all for plpgsql simple
expressions is that the cost of these checks, simple as they are,
is a noticeable fraction of the total time to do a simple expression.
That's not going to be the case for queries involving table access.

>> The comment is there because the regression tests fall over if you try
>> to do it the other way :-(.  The failure I saw was specific to a
>> transaction being done in a DO block, and maybe we could handle that
>> differently from the case for a normal procedure; but it seemed better
>> to me to make them the same.

> I'm still confused as to why it actually fixes the issue. Feel we should
> at least understand what's going on before commtting.

I do understand the issue.  If you make the simple-resowner a child
of TopTransactionResourceOwner, it vanishes at commit --- but
plpgsql_inline_handler has still got a pointer to it, which it'll try
to free afterwards, if the commit was inside the DO block.

What's not entirely clear to me is why this in exec_stmt_commit

@@ -4825,6 +4845,7 @@ exec_stmt_commit(PLpgSQL_execstate *estate, PLpgSQL_stmt_commit *stmt)
     }

     estate->simple_eval_estate = NULL;
+    estate->simple_eval_resowner = NULL;
     plpgsql_create_econtext(estate);

     return PLPGSQL_RC_OK;

is okay --- it avoids having a dangling pointer, sure, but if we're inside
a DO block won't plpgsql_create_econtext create a simple_eval_estate (and,
now, simple_eval_resowner) with the wrong properties?  But that's a
pre-existing question, and maybe Peter got it right and there's no
problem.

>> Doubt it.  On the other hand, as the code stands it's certain that the
>> resowner contains nothing but plancache pins (while I was writing the
>> patch it wasn't entirely clear that that would hold).  So we could
>> drop the two unnecessary calls.  There are assertions in
>> ResourceOwnerDelete that would fire if we somehow missed releasing
>> anything, so it doesn't seem like much of a maintenance hazard.

> One could even argue that that's a nice crosscheck: Due to the later
> release it'd not actually be correct to just add "arbitrary" things to
> that resowner.

OK, I'll change that.

>> (1) Not given the existing set of uses of the push/pop capability, which
>> so far as I can see is *only* CREATE SCHEMA.

> I do recall that there were issues with SET search_path in functions
> causing noticable slowdowns...

Yeah, possibly that could be improved, but that seems outside the scope of
this patch.

>> (2) as this is written, it's totally unsafe for the generation counter
>> ever to back up; that risks false match detections later.

> I was just thinking of backing up the 'active generation' state. New
> generations would have to come from a separate 'next generation'
> counter.

Oh, I see.  Yeah, that could work, but there's no point until we have
push/pop calls that are actually interesting for performance.

            regards, tom lane