Thread: Index build temp files

Greetings,
 We were surprised recently to note that the temp files that are created during a CREATE INDEX don't go into either a
temptablespace set for the user or into the tablespace which the CREATE INDEX specifies.  Instead, they go only into
base/pgsql_tmp/. This doesn't allow for any flexibility in defining where to create these potentially quite large sets
offiles.
 
 Shouldn't these temp files be going into the temp tablespace for the user creating the index instead..?  Or perhaps
intothe tablespace which the index is being created in?
 
     Thanks,
    Stephen

On Tue, Jan  8, 2013 at 05:09:47PM -0500, Stephen Frost wrote:
> Greetings,
> 
>   We were surprised recently to note that the temp files that are
>   created during a CREATE INDEX don't go into either a temp tablespace
>   set for the user or into the tablespace which the CREATE INDEX
>   specifies.  Instead, they go only into base/pgsql_tmp/.  This doesn't
>   allow for any flexibility in defining where to create these
>   potentially quite large sets of files.
> 
>   Shouldn't these temp files be going into the temp tablespace for the
>   user creating the index instead..?  Or perhaps into the tablespace
>   which the index is being created in?

Well, our docs for temp_tablespaces says:
       This variable specifies tablespaces in which to create temporary       objects (temp tables and indexes on temp
tables)when a       <command>CREATE</> command does not explicitly specify a tablespace.       Temporary files for
purposessuch as sorting large data sets       are also created in these tablespaces.
 

Are you saying this is inaccorate?

--  Bruce Momjian  <bruce@momjian.us>        http://momjian.us EnterpriseDB
http://enterprisedb.com
 + It's impossible for everything to be true. +

* Bruce Momjian (bruce@momjian.us) wrote:
> On Tue, Jan  8, 2013 at 05:09:47PM -0500, Stephen Frost wrote:
> > Greetings,
> >
> >   We were surprised recently to note that the temp files that are
> >   created during a CREATE INDEX don't go into either a temp tablespace
> >   set for the user or into the tablespace which the CREATE INDEX
> >   specifies.  Instead, they go only into base/pgsql_tmp/.  This doesn't
> >   allow for any flexibility in defining where to create these
> >   potentially quite large sets of files.
> >
> >   Shouldn't these temp files be going into the temp tablespace for the
> >   user creating the index instead..?  Or perhaps into the tablespace
> >   which the index is being created in?
>
> Well, our docs for temp_tablespaces says:
>
>         This variable specifies tablespaces in which to create temporary
>         objects (temp tables and indexes on temp tables) when a
>         <command>CREATE</> command does not explicitly specify a tablespace.
>         Temporary files for purposes such as sorting large data sets
>         are also created in these tablespaces.
>
> Are you saying this is inaccorate?

Yes and no?  Are the temporary files created during a CREATE INDEX
considered "Temporary files for purposes such as sorting large data
sets"?  My thinking is 'yes', but others may disagree.  Also,
considering this a bug would imply that it's back-patchable and I'm not
convinced it's worth the risk of breaking things which depend on the
current behavior.
Thanks,
    Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Bruce Momjian (bruce@momjian.us) wrote:
>> Well, our docs for temp_tablespaces says:
>> This variable specifies tablespaces in which to create temporary
>> objects (temp tables and indexes on temp tables) when a
>> <command>CREATE</> command does not explicitly specify a tablespace.
>> Temporary files for purposes such as sorting large data sets
>> are also created in these tablespaces.
>> 
>> Are you saying this is inaccorate?

> Yes and no?  Are the temporary files created during a CREATE INDEX
> considered "Temporary files for purposes such as sorting large data
> sets"?  My thinking is 'yes', but others may disagree.  Also,
> considering this a bug would imply that it's back-patchable and I'm not
> convinced it's worth the risk of breaking things which depend on the
> current behavior.

I don't think it's a bug.  What you seem to be proposing is that CREATE
INDEX ought to ignore temp_tablespaces and instead always put its temp
files in the tablespace where the finished index will reside.  This
would not be a good idea IMO --- allowing the temp files to be spread to
other tablespaces is better both from the standpoint of space usage and
the ability to overlap I/O.  (Admittedly, both of those concerns are
often theoretical, but if they are then I don't see why you'd care which
tablespace is used.)
        regards, tom lane

On 9 January 2013 00:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:

> I don't think it's a bug.  What you seem to be proposing is that CREATE
> INDEX ought to ignore temp_tablespaces and instead always put its temp
> files in the tablespace where the finished index will reside.

I don't think that's what he said.

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs <simon@2ndQuadrant.com> writes:
> On 9 January 2013 00:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I don't think it's a bug.  What you seem to be proposing is that CREATE
>> INDEX ought to ignore temp_tablespaces and instead always put its temp
>> files in the tablespace where the finished index will reside.

> I don't think that's what he said.

Then I misunderstood.  Stephen, what was your thought exactly about
what should happen?
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
> > On 9 January 2013 00:04, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I don't think it's a bug.  What you seem to be proposing is that CREATE
> >> INDEX ought to ignore temp_tablespaces and instead always put its temp
> >> files in the tablespace where the finished index will reside.
>
> > I don't think that's what he said.
>
> Then I misunderstood.  Stephen, what was your thought exactly about
> what should happen?

Perhaps this is more of a bug than I originally thought, given the
confusion and general expectation.

Here's what we're seeing:

postgresql.conf:
temp_tablespaces = 'temp_01,temp_02'

I have temp file logging on in postgresql.conf, so here's what we see:

LOG:  temporary file: path "base/pgsql_tmp/pgsql_tmp9221.4", size 204521472
CONTEXT:  SQL statement "create index table_key_idx on table (key) tablespace data_n04"

We observe the files being created under $DATA/base/pgsql_tmp/, ignoring
the temp tablespaces and not using the tablespace where the index is
ultimately created either.  I was speculating earlier that it should use
one or the other.  I'd be perfectly happy if it used one of the temp
tablespaces, but currently it's using pg_default regardless of the other
settings.
Thanks,
    Stephen

Stephen Frost <sfrost@snowman.net> writes:
> Here's what we're seeing:

> postgresql.conf:
> temp_tablespaces = 'temp_01,temp_02'

> I have temp file logging on in postgresql.conf, so here's what we see:

> LOG:  temporary file: path "base/pgsql_tmp/pgsql_tmp9221.4", size 204521472
> CONTEXT:  SQL statement "create index table_key_idx on table (key) tablespace data_n04"

> We observe the files being created under $DATA/base/pgsql_tmp/, ignoring
> the temp tablespaces and not using the tablespace where the index is
> ultimately created either.

Does the user running CREATE INDEX have CREATE permission on those
tablespaces?  (A quick way to double check is to try to SET
temp_tablespaces to that value explicitly.)  The code will silently
ignore any temp tablespaces you don't have such permission for.
        regards, tom lane

Tom,

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Does the user running CREATE INDEX have CREATE permission on those
> tablespaces?  (A quick way to double check is to try to SET
> temp_tablespaces to that value explicitly.)  The code will silently
> ignore any temp tablespaces you don't have such permission for.

Great point, thanks for that.  While working up a test case, I was able
to get it to use the temp tablespace.  In looking back at the actual
system, it turns out that the load process (which creates the indexes)
doesn't have rights on those temp tablespaces.

If I can come up with a doc or similar patch, I'll post it, but I have
to admit that the docs are pretty clear that you need create rights.
Still, it seems a little odd that we don't complain in any way in this
case.  Perhaps a NOTICE when the temp_tablespaces value is set but won't
be used due to permissions..?
Thanks,
    Stephen

On Tue, 2013-01-08 at 23:38 -0500, Stephen Frost wrote:
> If I can come up with a doc or similar patch, I'll post it, but I have
> to admit that the docs are pretty clear that you need create rights.
> Still, it seems a little odd that we don't complain in any way in this
> case.  Perhaps a NOTICE when the temp_tablespaces value is set but
> won't be used due to permissions..? 

It should be a warning in my opinion.

It should actually be an error, but there are probably reasons why that
wouldn't work.

Stephen Frost <sfrost@snowman.net> writes:
> If I can come up with a doc or similar patch, I'll post it, but I have
> to admit that the docs are pretty clear that you need create rights.
> Still, it seems a little odd that we don't complain in any way in this
> case.  Perhaps a NOTICE when the temp_tablespaces value is set but won't
> be used due to permissions..?

The code definitely will complain if you try to interactively SET
temp_tablespaces to a space you lack permissions for.  However, there
has never been a case in which people would hold still for warnings
emitted as a consequence of values read from postgresql.conf or other
background sources, and I doubt that the response would be different
if we made this variable act like that.  See for example past
discussions about what to do with invalid entries in search_path.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> The code definitely will complain if you try to interactively SET
> temp_tablespaces to a space you lack permissions for.  However, there
> has never been a case in which people would hold still for warnings
> emitted as a consequence of values read from postgresql.conf or other
> background sources, and I doubt that the response would be different
> if we made this variable act like that.  See for example past
> discussions about what to do with invalid entries in search_path.

Indeed, I fully expected the comparison argument to search_path, but I
have to admit that search_path feels a great deal more like CWD, while
the temp tablespaces is more like trying to write to /tmp and getting an
error.

That is to say, tablespaces and in particular temp tablespaces are much
more 'system' level than search paths.  I don't expect regular users to
change their temp tablepace, while I expect them to change their search
path on a regular basis.

In any case, I was suggesting a NOTICE rather than a WARNING, though I
appreciate that both could make noise for users who don't expect it.
Still, I don't expect many users would complain about this, while they
would complain about a similar thing for search_path.  Perhaps that's
not how they *should* act, but humans aren't always logical.  :)
Thanks,
    Stephen

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Does the user running CREATE INDEX have CREATE permission on those
> tablespaces?  (A quick way to double check is to try to SET
> temp_tablespaces to that value explicitly.)  The code will silently
> ignore any temp tablespaces you don't have such permission for.

Alright, this isn't quite as open-and-shut as it may have originally
seemed.  We're apparently cacheing the temp tablespaces which should be
used, even across set role's and security definer functions, which I
would argue isn't correct.

Attached are two test scripts and results from them.  The gist of it is
this:

With the first script, a privileged user creates a temp table and this
table ends up in a tablespace which that user has access to.  In the
same session, the user drops privileges using a 'set role' to a less
privileged user (who doesn't have access to the same tablespaces) and
then tries to create a temporary table- boom: permission denied error.

In the second script, an unprivileged user creates a temp table, which
goes into the default tablespace (this is fine- there are other temp
tablespaces, but this user doesn't have access to them), but then calls
a SECURITY DEFINER function, which runs as a privileged user who DOES
have access to the temp tablespaces defined by default.  What ends up
happening, however, is that the temporary table created during the
security definer function ends up going into the default tablespace
instead.  If the security definer function calls 'set temp_tablespaces',
before creating the temp table, it'll go into the correct tablespace but
after the security definer function ends, if the regular user tries to
create a temporary table they'll get a permission denied error.

I've not gone and looked at any of the code behind this yet (hopefully
will be able to soon).  It seems like we need to modify the cacheing
behavior of the temp tablespace code to account for the role that is
currently active.  This can't work quite like search_path since we want
to reuse the same tablespace, if possible, for the duration of a
session, per the docs.

Thoughts?
Thanks,
    Stephen

* Stephen Frost (sfrost@snowman.net) wrote:
> Attached are two test scripts and results from them.  The gist of it is
> this:

Now with the attachements.  Apologies about that.  Note that you'll need
to create the temp tablespace first.

    Thanks,

        Stephen

On 9 January 2013 02:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Stephen Frost <sfrost@snowman.net> writes:
>> Here's what we're seeing:
>
>> postgresql.conf:
>> temp_tablespaces = 'temp_01,temp_02'
>
>> I have temp file logging on in postgresql.conf, so here's what we see:
>
>> LOG:  temporary file: path "base/pgsql_tmp/pgsql_tmp9221.4", size 204521472
>> CONTEXT:  SQL statement "create index table_key_idx on table (key) tablespace data_n04"
>
>> We observe the files being created under $DATA/base/pgsql_tmp/, ignoring
>> the temp tablespaces and not using the tablespace where the index is
>> ultimately created either.
>
> Does the user running CREATE INDEX have CREATE permission on those
> tablespaces?  (A quick way to double check is to try to SET
> temp_tablespaces to that value explicitly.)  The code will silently
> ignore any temp tablespaces you don't have such permission for.

I think we need to allow a TEMP permission on tablespaces, so that you
aren't allowed to create normal objects but you can create TEMP
objects and sort files there.

I think SHOW temp_tablespaces should only show the valid tablespaces,
not all of the ones actually listed in the parameter, otherwise you
have no clue that the parameter setting is ineffectual for you.

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

Stephen Frost <sfrost@snowman.net> writes:
> Alright, this isn't quite as open-and-shut as it may have originally
> seemed.  We're apparently cacheing the temp tablespaces which should be
> used, even across set role's and security definer functions, which I
> would argue isn't correct.

Ah.  Yeah, that would be true.

We do have mechanism that forces search_path to be recomputed across
changes of active role, but it's expensive to do that, and it seems
of rather debatable value to do it here --- it certainly wouldn't
improve Stephen's original problem, much less the other issues he
raises here.

What would people think of just eliminating the access-permissions
checks involved in temp_tablespaces?  It would likely be appropriate to
change temp_tablespaces from USERSET to SUSET if we did so.  So
essentially the worldview would become that the DBA is responsible for
the temp_tablespaces setting, not individual users.
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> We do have mechanism that forces search_path to be recomputed across
> changes of active role, but it's expensive to do that, and it seems
> of rather debatable value to do it here --- it certainly wouldn't
> improve Stephen's original problem, much less the other issues he
> raises here.

I agree that we would need something more for users to be able to be
able to tell what temp tablespace their temporary tables will end up in.
Generally, I think it'd be acceptable from a performance perspective to
figure out where to create the temporary objects at the time that they
are requested- at that point, you're going to be creating a new table,
index, or doing a large sort that spills to disk, all of which are
relatively heavy-weight operations.

> What would people think of just eliminating the access-permissions
> checks involved in temp_tablespaces?  It would likely be appropriate to
> change temp_tablespaces from USERSET to SUSET if we did so.  So
> essentially the worldview would become that the DBA is responsible for
> the temp_tablespaces setting, not individual users.

I believe it's important to be able to control what temp tablespaces are
used on a per-user basis and that 'set role;' or security definer
functions respect the tablespace which has been set for the current
role.  Temp tablespaces are extremely valuable for managing users who
unintentionally write run-away queries that fill up the tablespace.
Keeping those seperate from the temp tablespace used for SECURITY
DEFINER functions (which are written with care) or other ongoing system
activity is necessary.  Perhaps there would be a way to still do that
with your approach, but it didn't sound like it initially.

Perhaps we can simply keep track of what the current role was when we
cache'd which temp tablespace was selected for a given session and, if
the current role has changed, reconsider the temp tablespace selected?
We would need to update the documentation to reflect that you might not
*always* have the same tablespace for a session, if there are security
definer and/or set role's happening, but that seems reasonable to me.
Thanks,
    Stephen

* Simon Riggs (simon@2ndQuadrant.com) wrote:
> I think we need to allow a TEMP permission on tablespaces, so that you
> aren't allowed to create normal objects but you can create TEMP
> objects and sort files there.

I agree that this would be valuable.  Would we end up needing to burn
another bit off the aclmask though?  That's certainly been a concern in
the past and I don't recall that we ever improved that situation.

> I think SHOW temp_tablespaces should only show the valid tablespaces,
> not all of the ones actually listed in the parameter, otherwise you
> have no clue that the parameter setting is ineffectual for you.

I like this as well.
Thanks,
    Stephen

Stephen Frost <sfrost@snowman.net> writes:
> * Tom Lane (tgl@sss.pgh.pa.us) wrote:
>> We do have mechanism that forces search_path to be recomputed across
>> changes of active role, but it's expensive to do that, and it seems
>> of rather debatable value to do it here --- it certainly wouldn't
>> improve Stephen's original problem, much less the other issues he
>> raises here.

> Generally, I think it'd be acceptable from a performance perspective to
> figure out where to create the temporary objects at the time that they
> are requested- at that point, you're going to be creating a new table,
> index, or doing a large sort that spills to disk, all of which are
> relatively heavy-weight operations.

It's not so much performance I'm worried about here as
modularity/layering issues.  Actual use of the temp-tablespaces list to
create temp files is in fd.c, which has no business invoking catalog
accesses, so we can't just say "we'll recheck the permissions when we're
going to create some temp files".  In any case this wouldn't improve the
behavior you were originally on about, which was surprising (to you)
failure to use a DBA-specified temp_tablespaces list.

>> What would people think of just eliminating the access-permissions
>> checks involved in temp_tablespaces?  It would likely be appropriate to
>> change temp_tablespaces from USERSET to SUSET if we did so.  So
>> essentially the worldview would become that the DBA is responsible for
>> the temp_tablespaces setting, not individual users.

> I believe it's important to be able to control what temp tablespaces are
> used on a per-user basis and that 'set role;' or security definer
> functions respect the tablespace which has been set for the current
> role.  Temp tablespaces are extremely valuable for managing users who
> unintentionally write run-away queries that fill up the tablespace.

IIRC, we do have mechanism now whereby a superuser can establish settings
for SUSET variables via ALTER ROLE SET/ALTER DATABASE SET, and
everything works as expected.  So the only loss of flexibility here
would be if you want unprivileged code to be able to set
temp_tablespaces for itself.  However, if you want that then it's hard
to argue that there shouldn't be a permissions check, and then we're
back into the surprises mentioned previously.

It might be possible to compromise on an arrangement whereby tablespace
permissions checking only occurs if the active value of the variable was
set by a non-superuser.  But TBH that idea fills me with dread --- we
don't have any other GUCs that work like that, and it seems too likely
that there would be conceptual or implementation bugs in it.
        regards, tom lane

On 9 January 2013 20:53, Stephen Frost <sfrost@snowman.net> wrote:
> * Simon Riggs (simon@2ndQuadrant.com) wrote:
>> I think we need to allow a TEMP permission on tablespaces, so that you
>> aren't allowed to create normal objects but you can create TEMP
>> objects and sort files there.
>
> I agree that this would be valuable.  Would we end up needing to burn
> another bit off the aclmask though?  That's certainly been a concern in
> the past and I don't recall that we ever improved that situation.

There already is a TEMP permission, it just only applies to databases
at present.

>> I think SHOW temp_tablespaces should only show the valid tablespaces,
>> not all of the ones actually listed in the parameter, otherwise you
>> have no clue that the parameter setting is ineffectual for you.
>
> I like this as well.


-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs <simon@2ndQuadrant.com> writes:
> On 9 January 2013 20:53, Stephen Frost <sfrost@snowman.net> wrote:
>> * Simon Riggs (simon@2ndQuadrant.com) wrote:
>>> I think we need to allow a TEMP permission on tablespaces, so that you
>>> aren't allowed to create normal objects but you can create TEMP
>>> objects and sort files there.

>> I agree that this would be valuable.  Would we end up needing to burn
>> another bit off the aclmask though?  That's certainly been a concern in
>> the past and I don't recall that we ever improved that situation.

> There already is a TEMP permission, it just only applies to databases
> at present.

This sounds like rather a lot of work to create a behavior that doesn't
solve the originally-complained-of usability problem.  All it does is
make things even more complicated, and add an extra step for the DBA
who's just trying to set temp_tablespaces to something useful.

(Or are you proposing that we'd grant TEMP permission to PUBLIC by
default?  We'd have compatibility issues with how to interpret the
grants in existing dump files if we do that.)
        regards, tom lane

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> IIRC, we do have mechanism now whereby a superuser can establish settings
> for SUSET variables via ALTER ROLE SET/ALTER DATABASE SET, and
> everything works as expected.

I'm not entirely convinced that it works as expected, at least for
temp_tablespaces currently.  This was a bit surprising to me:

=# alter user test_role set temp_tablespaces='zz';
NOTICE:  tablespace "zz" does not exist
ALTER ROLE
=*# commit;
COMMIT
=# set role test_role;
SET
=*> show temp_tablespaces;temp_tablespaces
------------------rn_temp_01
(1 row)

> So the only loss of flexibility here
> would be if you want unprivileged code to be able to set
> temp_tablespaces for itself.

I'm on the fence about this one.  I could see that being useful in some
cases when there are a lot of temporary tables being created and you
need more than one tablespace for simple space reasons.  It'd be
unfortuante if you had to be a superuser and/or call a superuser
security definer function to handle that situation.

> However, if you want that then it's hard
> to argue that there shouldn't be a permissions check, and then we're
> back into the surprises mentioned previously.

I'm not sure we're going to be able to get away from that.

> It might be possible to compromise on an arrangement whereby tablespace
> permissions checking only occurs if the active value of the variable was
> set by a non-superuser.  But TBH that idea fills me with dread --- we
> don't have any other GUCs that work like that, and it seems too likely
> that there would be conceptual or implementation bugs in it.

I don't particularly like that either.
Thanks,
    Stephen

On 9 January 2013 21:21, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> On 9 January 2013 20:53, Stephen Frost <sfrost@snowman.net> wrote:
>>> * Simon Riggs (simon@2ndQuadrant.com) wrote:
>>>> I think we need to allow a TEMP permission on tablespaces, so that you
>>>> aren't allowed to create normal objects but you can create TEMP
>>>> objects and sort files there.
>
>>> I agree that this would be valuable.  Would we end up needing to burn
>>> another bit off the aclmask though?  That's certainly been a concern in
>>> the past and I don't recall that we ever improved that situation.
>
>> There already is a TEMP permission, it just only applies to databases
>> at present.
>
> This sounds like rather a lot of work to create a behavior that doesn't
> solve the originally-complained-of usability problem.  All it does is
> make things even more complicated, and add an extra step for the DBA
> who's just trying to set temp_tablespaces to something useful.

There is already an extra step to GRANT the CREATE privilege, so how
does this change things?

The whole point of the thread is that that step was omitted.

Oracle (and IIRC others) allow tablespaces to be created as TEMP, so
that nobody can put normal objects in there. That is a useful facility
and there's no way currently in Postgres of setting things up like
that. Hence the proposal.

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs <simon@2ndQuadrant.com> writes:
> On 9 January 2013 21:21, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> This sounds like rather a lot of work to create a behavior that doesn't
>> solve the originally-complained-of usability problem.  All it does is
>> make things even more complicated, and add an extra step for the DBA
>> who's just trying to set temp_tablespaces to something useful.

> There is already an extra step to GRANT the CREATE privilege, so how
> does this change things?

The point is that it didn't occur to Stephen that putting
temp_tablespaces = 'foo, bar' into postgresql.conf should require
doing GRANT CREATE TO PUBLIC on those tablespaces in order to be
effective.  Changing the situation so that instead he needs to do
GRANT TEMP TO PUBLIC does not make it one whit more usable.

All that that will really accomplish is to break grant methods that are
working in (other people's) existing installations; ie if someone has
code that does know about the GRANT CREATE requirement, he will not
thank us if he suddenly has to spell it GRANT TEMP in the next release.

If we were designing this from scratch I'd agree that a separate TEMP
privilege would be a good thing.  But bolting one on now is likely
to create more problems than it fixes.  Particularly since it doesn't
actually fix any of the concrete problems enumerated in this thread.

I continue to think that getting rid of the privilege check would be
a more useful answer than changing which privilege is tested.
        regards, tom lane

On 9 January 2013 21:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> On 9 January 2013 21:21, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> This sounds like rather a lot of work to create a behavior that doesn't
>>> solve the originally-complained-of usability problem.  All it does is
>>> make things even more complicated, and add an extra step for the DBA
>>> who's just trying to set temp_tablespaces to something useful.
>
>> There is already an extra step to GRANT the CREATE privilege, so how
>> does this change things?
>
> The point is that it didn't occur to Stephen that putting
> temp_tablespaces = 'foo, bar' into postgresql.conf should require
> doing GRANT CREATE TO PUBLIC on those tablespaces in order to be
> effective.  Changing the situation so that instead he needs to do
> GRANT TEMP TO PUBLIC does not make it one whit more usable.
>
> All that that will really accomplish is to break grant methods that are
> working in (other people's) existing installations; ie if someone has
> code that does know about the GRANT CREATE requirement, he will not
> thank us if he suddenly has to spell it GRANT TEMP in the next release.
>
> If we were designing this from scratch I'd agree that a separate TEMP
> privilege would be a good thing.  But bolting one on now is likely
> to create more problems than it fixes.  Particularly since it doesn't
> actually fix any of the concrete problems enumerated in this thread.
>
> I continue to think that getting rid of the privilege check would be
> a more useful answer than changing which privilege is tested.

I wasn't suggesting that we test for TEMP instead of CREATE; what I
meant was we would test for CREATE *OR* TEMP to give more options for
management. Since CREATE is a powerful privilege, secure systems would
not wish to grant that to everyone, which is what I think caused the
issue, coupled with the inability to know whether temp_tablespaces is
set to something you have privileges on.

Your suggestion to make TEMP the default would be a useful way to
handle this, but its still the opposite of how things work now.

Granting CREATE by default on tablespaces is not a great plan.

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

Simon Riggs <simon@2ndQuadrant.com> writes:
> On 9 January 2013 21:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> If we were designing this from scratch I'd agree that a separate TEMP
>> privilege would be a good thing.  But bolting one on now is likely
>> to create more problems than it fixes.  Particularly since it doesn't
>> actually fix any of the concrete problems enumerated in this thread.
>> 
>> I continue to think that getting rid of the privilege check would be
>> a more useful answer than changing which privilege is tested.

> I wasn't suggesting that we test for TEMP instead of CREATE; what I
> meant was we would test for CREATE *OR* TEMP to give more options for
> management.

[ shrug... ]  That's weird, ie unlike the behavior of other privileges,
and it *still* doesn't fix any of the problems Stephen complained of.
        regards, tom lane

On 9 January 2013 22:09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> On 9 January 2013 21:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> If we were designing this from scratch I'd agree that a separate TEMP
>>> privilege would be a good thing.  But bolting one on now is likely
>>> to create more problems than it fixes.  Particularly since it doesn't
>>> actually fix any of the concrete problems enumerated in this thread.
>>>
>>> I continue to think that getting rid of the privilege check would be
>>> a more useful answer than changing which privilege is tested.
>
>> I wasn't suggesting that we test for TEMP instead of CREATE; what I
>> meant was we would test for CREATE *OR* TEMP to give more options for
>> management.
>
> [ shrug... ]  That's weird, ie unlike the behavior of other privileges,
> and it *still* doesn't fix any of the problems Stephen complained of.

I think we're having a disconnection half hour?

The privs could be seen as CREATE_ANY or CREATE_TEMP_ONLY. One is a
superset of the other, just like granting ANY is a superset of other
privs.

My view is it would fix the root cause of the problem, as explained.
If you wanted to make TEMP active by default, we can do that.

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
> > On 9 January 2013 21:42, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I wasn't suggesting that we test for TEMP instead of CREATE; what I
> > meant was we would test for CREATE *OR* TEMP to give more options for
> > management.
>
> [ shrug... ]  That's weird, ie unlike the behavior of other privileges,
> and it *still* doesn't fix any of the problems Stephen complained of.

Not realizing that I needed to grant privileges on the tablespace was my
oversight.  Having to grant privileges on a tablespace before users can
use it is reasonable, imv..  Displaying tablespaces that I don't have
access to through 'show temp_tablespaces;' doesn't feel quite right.
Having weird cases where we return permission denied errors or use the
incorrect tablespace are definitely a problem.
Thanks,
    Stephen

Simon Riggs <simon@2ndQuadrant.com> writes:
> On 9 January 2013 22:09, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> [ shrug... ]  That's weird, ie unlike the behavior of other privileges,
>> and it *still* doesn't fix any of the problems Stephen complained of.

> I think we're having a disconnection half hour?

> The privs could be seen as CREATE_ANY or CREATE_TEMP_ONLY. One is a
> superset of the other, just like granting ANY is a superset of other
> privs.

> My view is it would fix the root cause of the problem, as explained.

No; what it would do is provide a feature we don't currently have, ie
the ability to let unprivileged users select a specific tablespace for
temp files but not real tables.  That may or may not be a useful feature
(I do not recall any previous requests for it, and it's not immediately
obvious why someone should be allowed to create 100GB of temp files but
no not-temp files).  But it does *not* address either of the key points
that Stephen raised to begin with, which to my mind were:

(1) It's unintuitive that setting temp_tablespaces in postgresql.conf
isn't sufficient to make sessions use those tablespaces for temp files.
(This is a definitional problem.)

(2) The current coding of the permissions tests is actively wrong,
in that changes of role context in the backend don't affect what
happens.  (This is an implementation problem, but that doesn't mean
it's simple to fix.  Also, even if it acted as one might expect, there
would still be unfortunate usability issues in sessions that actually
do switch roles, as Stephen illustrated.)

Just checking a different permissions bit won't do a thing for either
of those problems.  I continue to suggest that the most effective fix
for those issues is to reconsider what we want the overall behavior
of temp_tablespaces to be in the first place.  If we consider it to be
controlled primarily by the DBA and not individual users, then we can
make both (1) and (2) go away very easily.
        regards, tom lane

On 9 January 2013 23:12, Tom Lane <tgl@sss.pgh.pa.us> wrote:

>> My view is it would fix the root cause of the problem, as explained.
>
> No; what it would do is provide a feature we don't currently have, ie
> the ability to let unprivileged users select a specific tablespace for
> temp files but not real tables.  That may or may not be a useful feature
> (I do not recall any previous requests for it, and it's not immediately
> obvious why someone should be allowed to create 100GB of temp files but
> no not-temp files).

Mixing real data with temp files confuses things and complicates
planning for physical backups, which would be able to simpy skip a
tablespace if it was marked as temp. I guess that's an argument for
explicitly marking tablespaces as TEMP, rather than making it a
privilege.

ALTER TABLESPACE my_temp_space SET (usage = 'temp');

Options for usage would be TEMP or ANY

> But it does *not* address either of the key points
> that Stephen raised to begin with, which to my mind were:
>
> (1) It's unintuitive that setting temp_tablespaces in postgresql.conf
> isn't sufficient to make sessions use those tablespaces for temp files.
> (This is a definitional problem.)

Please compare with search_path which suffers just the same.

> (2) The current coding of the permissions tests is actively wrong,
> in that changes of role context in the backend don't affect what
> happens.  (This is an implementation problem, but that doesn't mean
> it's simple to fix.  Also, even if it acted as one might expect, there
> would still be unfortunate usability issues in sessions that actually
> do switch roles, as Stephen illustrated.)

Agreed

> Just checking a different permissions bit won't do a thing for either
> of those problems.  I continue to suggest that the most effective fix
> for those issues is to reconsider what we want the overall behavior
> of temp_tablespaces to be in the first place.  If we consider it to be
> controlled primarily by the DBA and not individual users, then we can
> make both (1) and (2) go away very easily.

As mentioned in my original suggestions, the fact that SHOW returns a
false view of the situation is at least as relevant as the point
above.

I'm not unhappy if you want to change the default, as long as it is revokeable.

Anyway, enough from me.

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On Wed, Jan 09, 2013 at 03:20:33PM -0500, Tom Lane wrote:
> What would people think of just eliminating the access-permissions
> checks involved in temp_tablespaces?  It would likely be appropriate to
> change temp_tablespaces from USERSET to SUSET if we did so.  So
> essentially the worldview would become that the DBA is responsible for
> the temp_tablespaces setting, not individual users.

Allowing that the new behavior could be clearer, that gain is too small to
justify the application compatibility hazard of making temp_tablespaces SUSET.
I don't see something we can do here that clearly improves things overall.

On 10 January 2013 02:36, Noah Misch <noah@leadboat.com> wrote:
> On Wed, Jan 09, 2013 at 03:20:33PM -0500, Tom Lane wrote:
>> What would people think of just eliminating the access-permissions
>> checks involved in temp_tablespaces?  It would likely be appropriate to
>> change temp_tablespaces from USERSET to SUSET if we did so.  So
>> essentially the worldview would become that the DBA is responsible for
>> the temp_tablespaces setting, not individual users.
>
> Allowing that the new behavior could be clearer, that gain is too small to
> justify the application compatibility hazard of making temp_tablespaces SUSET.
> I don't see something we can do here that clearly improves things overall.

Can't we do both behaviours? Skip permissions if using a value form
.conf, but don't if the user sets it themselves.

Having it USERSET allows different settings for different roles, which
is useful.

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

On Thu, Jan 10, 2013 at 02:48:23AM +0000, Simon Riggs wrote:
> On 10 January 2013 02:36, Noah Misch <noah@leadboat.com> wrote:
> > On Wed, Jan 09, 2013 at 03:20:33PM -0500, Tom Lane wrote:
> >> What would people think of just eliminating the access-permissions
> >> checks involved in temp_tablespaces?  It would likely be appropriate to
> >> change temp_tablespaces from USERSET to SUSET if we did so.  So
> >> essentially the worldview would become that the DBA is responsible for
> >> the temp_tablespaces setting, not individual users.
> >
> > Allowing that the new behavior could be clearer, that gain is too small to
> > justify the application compatibility hazard of making temp_tablespaces SUSET.
> > I don't see something we can do here that clearly improves things overall.
> 
> Can't we do both behaviours? Skip permissions if using a value form
> .conf, but don't if the user sets it themselves.

We could, though I share Tom's reluctance[1].

[1] http://archives.postgresql.org/message-id/1985.1357765424@sss.pgh.pa.us

Simon Riggs <simon@2ndQuadrant.com> writes:
> Having it USERSET allows different settings for different roles, which
> is useful.

That would still be possible if it were SUSET; you'd just need a
superuser to set it for you (via ALTER ROLE SET, or perhaps a
security-definer wrapper function if you were desperate).

The real question is how necessary is it for unprivileged code to set
temp_tablespaces *for itself*.  I doubt that that's all that critical.
I suspect the variable is hardly used in the field at all, given that
it's been there since 8.3 and nobody noticed these issues until
Stephen started poking at it.

Note I am certainly not suggesting that we back-patch any such change.
This would just be for 9.3 and up.  We have surely made bigger
incompatible changes than this one, probably in just about every major
release.
        regards, tom lane

On 10 January 2013 04:02, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Simon Riggs <simon@2ndQuadrant.com> writes:
>> Having it USERSET allows different settings for different roles, which
>> is useful.
>
> That would still be possible if it were SUSET; you'd just need a
> superuser to set it for you (via ALTER ROLE SET, or perhaps a
> security-definer wrapper function if you were desperate).
>
> The real question is how necessary is it for unprivileged code to set
> temp_tablespaces *for itself*.  I doubt that that's all that critical.

OK, I can accept that

> I suspect the variable is hardly used in the field at all, given that
> it's been there since 8.3 and nobody noticed these issues until
> Stephen started poking at it.

...just not that bit, since it was not being used as documented.

> Note I am certainly not suggesting that we back-patch any such change.
> This would just be for 9.3 and up.  We have surely made bigger
> incompatible changes than this one, probably in just about every major
> release.

OK

-- Simon Riggs                   http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services

* Tom Lane (tgl@sss.pgh.pa.us) wrote:
> The real question is how necessary is it for unprivileged code to set
> temp_tablespaces *for itself*.  I doubt that that's all that critical.
> I suspect the variable is hardly used in the field at all, given that
> it's been there since 8.3 and nobody noticed these issues until
> Stephen started poking at it.

I'd like to outline a bit of how we got here and what caused us (at
$work) to discover the issue.  Perhaps that will shed some light on the
usefulness of the parameter and how it's used in the field.

We don't currently explicitly set 'temp_tablespaces' anywhere except in
the postgresql.conf.

We have a pretty large ETL process which logs in as an ETL user and
loads the data and then calls a bunch of plpgsql code to process it.
The ETL user just has rights to load the data and we specifically don't
want the ETL user to be able to create objects in the temp tablespaces.
The temp tablespaces, in our view, are intended to exclusively be *only*
temporary objects which don't need to be backed up, etc.  We can't
enforce that currently but we would like to.  The plpgsql code runs as
security definer and as a more privileged role which does have rights
to the temp tablespaces and we expect that to happen.  This more
privileged role certainly is *not* superuser and we'd want to keep it
that way.  I can see how we could have a function to call before
creating any objects as this user which runs as superuser to set
temp_tablespaces and then to un-set it later when we exit the function,
but that's a maintenance headache and really feels more complicated
than it should be.  Even if it didn't require superuser, going through
and littering the code with places where we have to set temp_tablespace
wouldn't be fun.

Being able to, as a superuser, set the variable for this user to use
these temp tablespaces and another user to use other temp tablespaces
seems like it would work, but it would need to correctly update
temp_tablespaces across security definer and set role's to do what we're
looking for.  It's not clear from the proposal being put forth as to if
that would happen or not.

On another system, we actually have things reversed, where we have
temp_tablespaces set in postgresql.conf, but most of the users on the
system have access to those temp_tablespaces and we expect/want them to
use the ones we set, to keep them from filling up the disk that the
default tablespace is on with large queries.  Being able to *restrict*
them from using the main tablespace for temporary objects could be nice
there, though we haven't run into any issues (they're not terribly
savvy wrt PG, though I could see that changing if we ended up showing
them how to set temp_tablespaces for themselves for some reason..).  We
have run into issues where they've created non-temporary objects in the
temporary tablespaces before though (they're familiar with being able to
specify a tablespace in a 'create table' command or, worse really,
through pgAdmin that a lot of these users use).  Generally we can get
away with "well, don't do that", but I do suspect it's only a matter of
time until something happens and someone loses data.

As relates to my initial complaint, I'll be honest, I've not used temp
tablespaces all that much previously.  My initial thought was that
'create index's temp files simply didn't respect temp_tablespaces and
so I was just trying to quickly create a test case to post with.  I do
agree with Simon that it'd be nice if the user was made aware somehow
about permissions issues regarding temp_tablespaces, presuming we keep
the permission system.  Also, for my part, I fully expected something
more like 'create temporary tablespace' rather than a postgresql.conf
setting that tried to act like search_path, but not really because it
does a round-robin on a per-session basis instead of a linear search,
explicitly has the default tablespace added to it (which I don't
consider to be anything like how search_path and pg_catalog work- you
don't create objects in pg_catalog) and isn't kept track of the way
search_path is wrt set role and security definer functions.  Indeed,
having it behave the way it does makes me worried that we'd need to deal
with it for security definer functions in the same way that we do search
path and that has never felt like a terribly good situation to me.

Apologies for having a bunch of complaints rather than solutions.  I
continue to wonder about simply tracking the role that the temp
tablespace look-up was done as and re-doing that lookup if the role has
changed.  That would keep much of the existing semantics (where they
make sense) and the permissions system.  Debate about adding other flags
or grants on tablespaces to be denoted as for-temp-objects-only could be
an independent feature proposal.
Thanks,
    Stephen