At Tue, 12 Nov 2019 11:27:24 +0300, Konstantin Knizhnik
<k.knizh...@postgrespro.ru> wrote in
>
>
> 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 agree for just relying on contain_mutable_functions for the same
reasons to Tom.
> 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.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
