Обсуждение: Prevent extension creation in temporary schemas

Hi all,

While looking at another bug I have noticed that it is possible to
create an extension directly using a temporary schema, which is
crazy.  A simple example:
=# create extension pg_prewarm with schema pg_temp_3;
CREATE EXTENSION
=# \dx pg_prewarm
           List of installed extensions
    Name    | Version |  Schema   |      Description
------------+---------+-----------+-----------------------
 pg_prewarm | 1.2     | pg_temp_3 | prewarm relation data
(1 row)

When also creating some extensions, like pageinspect, then the error
message gets a bit crazier, complaining about things not existing.
This combination makes no actual sense, so wouldn't it be better to
restrict the case?  When trying to use ALTER EXTENSION SET SCHEMA we
already have a similar error:
=# alter extension pageinspect set schema pg_temp_3;
ERROR:  0A000: cannot move objects into or out of temporary schemas
LOCATION:  CheckSetNamespace, namespace.c:2954

Attached is an idea of patch, the test case is a bit bulky to remain
portable though.

Thoughts?
--
Michael

On Sun, Jan 6, 2019 at 10:26 PM Michael Paquier <michael@paquier.xyz> wrote:
> This combination makes no actual sense, so wouldn't it be better to
> restrict the case?

Hmm.  What exactly doesn't make sense about it?

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

On Fri, Jan 11, 2019 at 02:22:01PM -0500, Robert Haas wrote:
> On Sun, Jan 6, 2019 at 10:26 PM Michael Paquier <michael@paquier.xyz> wrote:
>> This combination makes no actual sense, so wouldn't it be better to
>> restrict the case?
>
> Hmm.  What exactly doesn't make sense about it?

In my mind, extensions are designed to be database-wide objects which
are visible to all sessions.  Perhaps I am just confused by what I
think they should be, and I can see no trace on the archives about
concept of extensions + temp schema as base (adding Dimitri in CC if
he has an idea).  I can see as well that there have been stuff about
using temporary objects in extension script though ("Fix bugs with
temporary or transient tables used in extension scripts" in release
notes of 9.1).

For most of extensions, this can randomly finish with strange error
messages, say that:
=# create extension file_fdw with schema pg_temp_3;
ERROR:  42883: function file_fdw_handler() does not exist
LOCATION:  LookupFuncName, parse_func.c:2088

There are cases where the extension can be created:
=# create extension pgcrypto with schema pg_temp_3;
CREATE EXTENSION
Time: 36.567 ms
=# \dx pgcrypto
  List of installed extensions
   Name   | Version |  Schema   |       Description
----------+---------+-----------+-------------------------
 pgcrypto | 1.3     | pg_temp_3 | cryptographic functions
(1 row)

Then the extension is showing up as beginning to be present for other
users.  I am mainly wondering if this case has actually been thought
about in the past or discussed, and what to do about that and if we
need to do something.  Temporary extensions can exist as long as the
extension script does not include for example REVOKE queries on the
functions it creates (which should actually work?), and there is a
separate thread about restraining 2PC when touching the temporary
namespace for the creation of many objects, and extensions are one
case discussed.  Still the concept looks a bit wider, so I spawned a
separate thread.
--
Michael

On Sat, Jan 12, 2019 at 08:34:37AM +0900, Michael Paquier wrote:
> Then the extension is showing up as beginning to be present for other
> users.  I am mainly wondering if this case has actually been thought
> about in the past or discussed, and what to do about that and if we
> need to do something.

The point here is about the visibility in \dx.
--
Michael

This could probably use a quick note in the docs.

On Wed, Feb 13, 2019 at 12:08:50PM +0100, Chris Travers wrote:
> If the point is visibility in \dx it seems to me we want to fix the \dx
> query.

Yes, I got to think a bit more about that case, and there are cases
where this actually works properly as this depends on the objects
defined in the extension.  Fixing \dx to not show up extensions
defined in temp schemas of other sessions is definitely a must in my
opinion, and I would rather drop the rest of the proposal for now.  A
similar treatment is needed for \dx+.

> For example: suppose I create a type in pg_temp and create a table in
> public with a column using that type.

I am wondering if this scenario could make sense to populate data on
other, existing, relations for a schema migration, and that a two-step
process is done, with temporary tables used as intermediates.  But
that sounds like the thoughts of a crazy man..

> What is the expected visibility in other sessions?
>
> What happens to the table when I log out?

Anything depending on a temporary object will be dropped per
dependency links once the session is over.

Attached is a patch to adjust \dx and \dx+.  What do you think?
--
Michael

Dear Michael,

I seem this patch is enough, but could you explain the reason 
you drop initial proposal more detail?
I'm not sure why extensions contained by temporary schemas are acceptable.

> Anything depending on a temporary object will be dropped per
> dependency links once the session is over.

Extensions locate at pg_temp_* schemas are temporary objects IMO.
How do you think? Would you implement this functionality in future?

Hayato Kuroda
Fujitsu LIMITED

On Thu, Feb 14, 2019 at 4:57 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Feb 13, 2019 at 12:08:50PM +0100, Chris Travers wrote:
> > If the point is visibility in \dx it seems to me we want to fix the \dx
> > query.
>
> Yes, I got to think a bit more about that case, and there are cases
> where this actually works properly as this depends on the objects
> defined in the extension.  Fixing \dx to not show up extensions
> defined in temp schemas of other sessions is definitely a must in my
> opinion, and I would rather drop the rest of the proposal for now.  A
> similar treatment is needed for \dx+.

I'd vote for accepting the extension creation in temporary schemas and
fixing \dx and \dx+. However the error raised by creating extensions
in temporary schema still looks strange to me. Since we don't search
functions and operators defined in temporary schemas  (which is stated
by the doc) unless we use qualified function name we cannot create
extensions in temporary schema whose functions refer theirs other
functions. I'd like to fix it or to find a workaround but cannot come
up with a good idea yet.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On Mon, Feb 18, 2019 at 05:39:09AM +0000, Kuroda, Hayato wrote:
> I seem this patch is enough, but could you explain the reason
> you drop initial proposal more detail?
> I'm not sure why extensions contained by temporary schemas are
> acceptable.

Because there are cases where they actually work.  We have some of
these in core.

>> Anything depending on a temporary object will be dropped per
>> dependency links once the session is over.
>
> Extensions locate at pg_temp_* schemas are temporary objects IMO.
> How do you think? Would you implement this functionality in future?

Per the game of dependencies, extensions located in a temporary schema
would get automatically dropped at session end.
--
Michael

On Mon, Feb 18, 2019 at 08:02:54PM +0900, Masahiko Sawada wrote:
> I'd vote for accepting the extension creation in temporary schemas and
> fixing \dx and \dx+.

Thanks.

> However the error raised by creating extensions
> in temporary schema still looks strange to me. Since we don't search
> functions and operators defined in temporary schemas  (which is stated
> by the doc) unless we use qualified function name we cannot create
> extensions in temporary schema whose functions refer theirs other
> functions. I'd like to fix it or to find a workaround but cannot come
> up with a good idea yet.

Agreed.  Getting a schema mismatch is kind of disappointing, and it
depends on the DDL used in the extension SQL script.  I would suspect
that getting that addressed correctly may add quite some facility, for
little gain.  But I may be wrong, that's only the feeling coming from
a shiver in my back.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Mon, Feb 18, 2019 at 05:39:09AM +0000, Kuroda, Hayato wrote:
>> I'm not sure why extensions contained by temporary schemas are
>> acceptable.

> Because there are cases where they actually work.

More to the point, it doesn't seem that hard to think of cases
where this would be useful.  PG extensions are very general
things.  If you want to create a whole pile of temporary objects
and do that repeatedly, wrapping them up into an extension is
a nice way to do that, nicer really than anything else we offer.
So I'd be sad if we decided to forbid this.

> Per the game of dependencies, extensions located in a temporary schema
> would get automatically dropped at session end.

Yeah, it doesn't seem like there's actually any missing functionality
there, at least not any that's specific to extensions.

            regards, tom lane

Dear Michael, Chris and Tom,

> Adding special cases to extensions strikes me as adding more
> funny corners to the behavior of the db in this regard.

I understand your arguments and its utility.

> For most of extensions, this can randomly finish with strange error
> messages, say that:
> =# create extension file_fdw with schema pg_temp_3;
> ERROR:  42883: function file_fdw_handler() does not exist
> LOCATION:  LookupFuncName, parse_func.c:2088

I found that this strange error appears after making
temporary tables. 

test=> CREATE TEMPORARY TABLE temp (id int);
CREATE TABLE
test=> CREATE EXTENSION file_fdw WITH SCHEMA pg_temp_3;
ERROR:  function file_fdw_handler() does not exist

I would try to understand this problem for community and
my experience.

Best Regards,
Hayato Kuroda
Fujitsu LIMITED

Hi

> I found that this strange error appears after making
> temporary tables.
>
> test=> CREATE TEMPORARY TABLE temp (id int);
> CREATE TABLE
> test=> CREATE EXTENSION file_fdw WITH SCHEMA pg_temp_3;
> ERROR: function file_fdw_handler() does not exist
>
> I would try to understand this problem for community and
> my experience.

This behavior seems as not related to extensions infrastructure:

postgres=#  CREATE TEMPORARY TABLE temp (id int);
CREATE TABLE
postgres=# set search_path to 'pg_temp_3';
SET
postgres=# create function foo() returns int as 'select 1' language sql;
CREATE FUNCTION
postgres=# select pg_temp_3.foo();
 foo 
-----
   1
(1 row)

postgres=# select foo();
ERROR:  function foo() does not exist
LINE 1: select foo();
               ^
HINT:  No function matches the given name and argument types. You might need to add explicit type casts.
postgres=# show search_path ;
 search_path 
-------------
 pg_temp_3
(1 row)

regards, Sergei

Sergei Kornilov <sk@zsrv.org> writes:
>> test=> CREATE EXTENSION file_fdw WITH SCHEMA pg_temp_3;
>> ERROR: function file_fdw_handler() does not exist

> This behavior seems as not related to extensions infrastructure:

Yeah, I think it's just because we won't search the pg_temp schema
for function or operator names, unless the calling SQL command
explicitly writes "pg_temp.foo(...)" or equivalent.  That's an
ancient security decision, which we're unlikely to undo.  It
certainly puts a crimp in the usefulness of putting extensions into
pg_temp, but I don't think it totally destroys the usefulness.
You could still use an extension to package, say, the definitions
of a bunch of temp tables and views that you need to create often.

            regards, tom lane

On Thu, Feb 28, 2019 at 10:13:17AM -0500, Tom Lane wrote:
> Yeah, I think it's just because we won't search the pg_temp schema
> for function or operator names, unless the calling SQL command
> explicitly writes "pg_temp.foo(...)" or equivalent.  That's an
> ancient security decision, which we're unlikely to undo.  It
> certainly puts a crimp in the usefulness of putting extensions into
> pg_temp, but I don't think it totally destroys the usefulness.
> You could still use an extension to package, say, the definitions
> of a bunch of temp tables and views that you need to create often.

Even with that, it should still be possible to enforce search_path
within the extension script to allow such objects to be created
correctly, no?  That would be a bit hacky, still for the purpose of
temp object handling that looks kind of enough to live with when
creating an extension.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Feb 28, 2019 at 10:13:17AM -0500, Tom Lane wrote:
>> Yeah, I think it's just because we won't search the pg_temp schema
>> for function or operator names, unless the calling SQL command
>> explicitly writes "pg_temp.foo(...)" or equivalent.  That's an
>> ancient security decision, which we're unlikely to undo.  It
>> certainly puts a crimp in the usefulness of putting extensions into
>> pg_temp, but I don't think it totally destroys the usefulness.
>> You could still use an extension to package, say, the definitions
>> of a bunch of temp tables and views that you need to create often.

> Even with that, it should still be possible to enforce search_path
> within the extension script to allow such objects to be created
> correctly, no?  That would be a bit hacky, still for the purpose of
> temp object handling that looks kind of enough to live with when
> creating an extension.

If you're suggesting that we disable that security restriction
during extension creation, I really can't see how that'd be a
good thing ...

            regards, tom lane

On Thu, Feb 28, 2019 at 10:52:52PM -0500, Tom Lane wrote:
> If you're suggesting that we disable that security restriction
> during extension creation, I really can't see how that'd be a
> good thing ...

No, I don't mean that.  I was just wondering if someone can set
search_path within the SQL script which includes the extension
contents to bypass the restriction and the error.  They can always
prefix such objects with pg_temp anyway if need be...
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> On Thu, Feb 28, 2019 at 10:52:52PM -0500, Tom Lane wrote:
>> If you're suggesting that we disable that security restriction
>> during extension creation, I really can't see how that'd be a
>> good thing ...

> No, I don't mean that.  I was just wondering if someone can set
> search_path within the SQL script which includes the extension
> contents to bypass the restriction and the error.  They can always
> prefix such objects with pg_temp anyway if need be...

You'd have to look in namespace.c to be sure, but I *think* that
we don't consult the temp schema during function/operator lookup
even if it's explicitly listed in search_path.

It might be possible for an extension script to get around this with
code like, say,

CREATE TRIGGER ... EXECUTE PROCEDURE @extschema@.myfunc();

although you'd have to give up relocatability of the extension
to use @extschema@.  (Maybe it was a bad idea to not provide
that symbol in relocatable extensions?  A usage like this doesn't
prevent the extension from being relocated later.)

            regards, tom lane

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:       tested, passed
Spec compliant:           tested, passed
Documentation:            not tested

I ran make checkworld and everything passed.

I tried installing a test extension into a temp schema.  I found this was remarkably difficult to do because pg_temp
didnot work (I had to create a temporary table and then locate the actual table it was created in).  While that might
alsobe a bug it is not in the scope of this patch so mostly noting in terms of future work.
 

After creating the extension I did as follows:
\dx in the current session shows the extension
\dx in a stock psql shows the extension in a separate session
\dx with a patched psql in a separate session does not show the extension.

In terms of the scope of this patch, I think this correctly and fully solves the problem at hand.

The new status of this patch is: Ready for Committer

On Tue, Mar 05, 2019 at 12:47:54PM +0000, Chris Travers wrote:
> I tried installing a test extension into a temp schema.  I found
> this was remarkably difficult to do because pg_temp did not work (I
> had to create a temporary table and then locate the actual table it
> was created in).  While that might also be a bug it is not in the
> scope of this patch so mostly noting in terms of future work.

pgcrypto works in this case.

> After creating the extension I did as follows:
> \dx in the current session shows the extension
> \dx in a stock psql shows the extension in a separate session
> \dx with a patched psql in a separate session does not show the
> extension.
>
> In terms of the scope of this patch, I think this correctly and
> fully solves the problem at hand.

I was just looking at this patch this morning with fresh eyes, and I
think that I have found one argument to *not* apply it.  Imagine the
following in one session:
=# create extension pgcrypto with schema pg_temp_3;
CREATE EXTENSION
=# \dx
                  List of installed extensions
   Name   | Version |   Schema   |         Description
----------+---------+------------+------------------------------
 pgcrypto | 1.3     | pg_temp_3  | cryptographic functions
 plpgsql  | 1.0     | pg_catalog | PL/pgSQL procedural language
(2 rows)

That's all good, we see that the session which created this extension
has it listed.  Now let's use in parallel a second session:
=# create extension pgcrypto with schema pg_temp_4;
ERROR:  42710: extension "pgcrypto" already exists
LOCATION:  CreateExtension, extension.c:1664
=# \dx
                  List of installed extensions
   Name   | Version |   Schema   |         Description
----------+---------+------------+------------------------------
 plpgsql  | 1.0     | pg_catalog | PL/pgSQL procedural language
(1 row)

This is actually also good, because the extension of the temporary
schema of the first session does not show up.  Now I think that this
can bring some confusion to the user actually, because the extension
becomes not listed via \dx, but trying to create it with a different
schema fails.

Thoughts?
--
Michael

On Wed, Mar 06, 2019 at 09:33:55AM +0100, Chris Travers wrote:
> Ok so at present I see three distinct issues here, where maybe three
> different patches over time might be needed.
>
> The issues are:
>
> 1.  create extension pgcrypto with schema pg_temp; fails because there is
> no schema actually named pg_temp.

Yes, I agree that being able to accept pg_temp as an alias for the
temporary schema for extensions would be kind of nice.  Perhaps one
reason why this has not actually happened is that out user base does
not really have use cases for it though.

> 2.  If you work around this, the \dx shows temporary extensions in other
> sessions.  This is probably the most minor issue of the three.
> 3.  You cannot create the same extension in two different schemas.

I would like to think that it should be possible to create the same
extension linked to a temporary schema in multiple sessions in
parallel, as much as it is possible to create the same extension
across multiple schemas.  Both are actually linked as temp schemas as
based on connection slots.  This would require some changes in the way
constraints are defined in catalogs for extensions.  Perhaps there is
either no demand for it, I don't know.

> But I don't think there is likely to be a lot of user confusion here.  It
> is hard enough to install extensions in temporary schemas that those who do
> can be expected to know more that \dx commands.

The patch as it stands does not actually solve the root problem and
makes things a bit confusing, so I am marking it as returned with
feedback.  Working on this set of problems may be interesting, though
the efforts necessary to make that may not be worth the actual user
benefits.
--
Michael