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
