On 11.11.2019 20:22, Tom Lane wrote:
>
> None of those statements are true, in my experience.
>
> In general, this patch seems like it's learned nothing from our
> experiences with the late and unlamented exec_simple_check_node()
> (cf commit 00418c612). Having a piece of plpgsql that has to know
> about all possible "simple expression" node types is just a major
> maintenance headache; but there is no short-cut solution that is
> really going to be acceptable. Either you're unsafe, or you fail
> to optimize cases that users will complain about, or you write
> and maintain a lot of code.
>
> I'm also pretty displeased with the is-it-in-the-pg_catalog-schema
> tests. Those are expensive, requiring additional catalog lookups,
> and they prove just about nothing. There are extensions that shove
> stuff into pg_catalog (look no further than contrib/adminpack), or
> users could do it, so you really still are relying on the whole world
> to get immutability right. If we're going to not trust immutability
> markings on user-defined objects, I'd be inclined to do it by
> checking that the object's OID is less than FirstGenbkiObjectId.
>
> But maybe we should just forget that bit of paranoia and rely solely
> on contain_mutable_functions(). That gets rid of the new maintenance
> requirement, and I think arguably it's OK. An "immutable" function
> whose result depends on changes that were just made by the calling
> function is pretty obviously mislabeled, so people won't have much of
> a leg to stand on if they complain about that. Pavel's argument
> upthread that people sometimes intentionally mislabel functions as
> immutable isn't really relevant: in his example, at least, the user
> is intentionally trying to get the function to be evaluated early.
> So whether it sees the effects of changes just made by the calling
> function doesn't seem like it would matter to such a user.
>
> regards, tom lane
In my opinion contain_mutable_functions() is the best solution.
But if it is not acceptable, I will rewrite the patch in white-list fashion.
I do not understand the argument about expensive
is-it-in-the-pg_catalog-schema test.
IsCatalogNameaspace is doing just simple comparison without any catalog
lookups:
bool
IsCatalogNamespace(Oid namespaceId)
{
return namespaceId == PG_CATALOG_NAMESPACE;
}
I may agree that it "proves nothing" if extension can put stuff in
pg_catalog.
I can replace it with comparison with FirstGenbkiObjectId.
But all this makes sense only if using contain_mutable_function() is not
acceptable.
--
Konstantin Knizhnik
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company
