Re: [HACKERS] relpersistence and temp table
Robert Haas wrote: On Fri, Jul 1, 2011 at 10:32 AM, Robert Haas robertmh...@gmail.com wrote: On Fri, Jul 1, 2011 at 8:06 AM, Amit Khandekar amit.khande...@enterprisedb.com wrote: In 9.1, if a table is created using an explicit pg_temp qualification, the pg_class.relpersistence is marked 'p', not 't'. That's a bug. ?Thanks for the report. OK, so I think the problem here is that, in 9.0, it was possible to figure out what value relistemp should take at a very late date, because it was entirely a function of the schema name. A temporary schema implies relistemp = true, while a non-temporary schema implies relistemp = false. However, in 9.1, that clearly won't do, since unlogged and permanent tables can share the same schema. Moreover, by the time we get as far as RelationBuildLocalRelation(), we've already made lots of other decisions based on relpersistence, so it seems that we need to make this correct as early as possible. It's not feasible to do that in the parser, because the creation namespace could also come from search_path: SET search_path = pg_temp; CREATE TABLE foo (a int); So it seems we can't fix this any earlier than RangeVarGetCreationNamespace(). In the attached patch, I took basically that approach, but created a new function RangeVarAdjustRelationPersistence() that does the actual adjusting (since de-constifying RangeVarGetCreationNamespace() didn't seem smart), plus adds a bunch of additional sanity-checking that I previously overlooked. Namely, it forbids: - creating unlogged tables in temporary schemas - creating relations in temporary schemas of other sessions On the other hand, it does allow CREATE TEMP TABLE pg_temp.foo(a int), which was somewhat pointlessly forbidden by previous releases. In short, the code now checks directly what it used to check by inference: that you're not creating a temporary table in a permanent schema, or the other way around. I also rearranged a few other bits of code to make sure that the appropriate fixups happen BEFORE we enforce the condition that temporary tables mustn't be created in security-restricted contexts. Does this affect tables created during 9.1 beta? I assume a server restart fixes all this, but I am just checking. -- Bruce Momjian br...@momjian.ushttp://momjian.us EnterpriseDB http://enterprisedb.com + It's impossible for everything to be true. + -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] relpersistence and temp table
On Jul 11, 2011, at 8:55 PM, Bruce Momjian br...@momjian.us wrote: Does this affect tables created during 9.1 beta? I assume a server restart fixes all this, but I am just checking. Yes, I think a server restart will fix it, though there might be corner cases I'm not thinking of. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] relpersistence and temp table
On Fri, Jul 1, 2011 at 1:59 PM, Robert Haas robertmh...@gmail.com wrote: On Fri, Jul 1, 2011 at 10:32 AM, Robert Haas robertmh...@gmail.com wrote: On Fri, Jul 1, 2011 at 8:06 AM, Amit Khandekar amit.khande...@enterprisedb.com wrote: In 9.1, if a table is created using an explicit pg_temp qualification, the pg_class.relpersistence is marked 'p', not 't'. That's a bug. Thanks for the report. OK, so I think the problem here is that, in 9.0, it was possible to figure out what value relistemp should take at a very late date, because it was entirely a function of the schema name. A temporary schema implies relistemp = true, while a non-temporary schema implies relistemp = false. However, in 9.1, that clearly won't do, since unlogged and permanent tables can share the same schema. Moreover, by the time we get as far as RelationBuildLocalRelation(), we've already made lots of other decisions based on relpersistence, so it seems that we need to make this correct as early as possible. It's not feasible to do that in the parser, because the creation namespace could also come from search_path: SET search_path = pg_temp; CREATE TABLE foo (a int); So it seems we can't fix this any earlier than RangeVarGetCreationNamespace(). In the attached patch, I took basically that approach, but created a new function RangeVarAdjustRelationPersistence() that does the actual adjusting (since de-constifying RangeVarGetCreationNamespace() didn't seem smart), plus adds a bunch of additional sanity-checking that I previously overlooked. Namely, it forbids: - creating unlogged tables in temporary schemas - creating relations in temporary schemas of other sessions On the other hand, it does allow CREATE TEMP TABLE pg_temp.foo(a int), which was somewhat pointlessly forbidden by previous releases. In short, the code now checks directly what it used to check by inference: that you're not creating a temporary table in a permanent schema, or the other way around. I also rearranged a few other bits of code to make sure that the appropriate fixups happen BEFORE we enforce the condition that temporary tables mustn't be created in security-restricted contexts. Committed. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] relpersistence and temp table
On Fri, Jul 1, 2011 at 8:06 AM, Amit Khandekar amit.khande...@enterprisedb.com wrote: In 9.1, if a table is created using an explicit pg_temp qualification, the pg_class.relpersistence is marked 'p', not 't'. postgres=# CREATE TABLE pg_temp.temptable (i int4); CREATE TABLE postgres=# select relpersistence from pg_class where relname = 'temptable'; relpersistence p (1 row) BUt the table does go away if I exit the session: postgres=# \q edb-pg ~/git-pg1 $ psql psql (9.0.1, server 9.2devel) WARNING: psql version 9.0, server version 9.2. Some psql features might not work. Type help for help. postgres=# select relpersistence from pg_class where relname = 'temptable'; relpersistence (0 rows) So in that sense, it does work as a temp table, but the relpersistence is not 't'. So, is the tempness no longer identified by pg_class.relpersistence now? In RelationBuildLocalRelation(), previously, namespace was used to determine if the table should be marked temporary: /* it is temporary if and only if it is in my temp-table namespace */ rel-rd_istemp = isTempOrToastNamespace(relnamespace); But in Master branch, now if I look at RelationBuildLocalRelation(), there is no such logic to mark relpersistence. Was this intentional or is it a bug? That's a bug. Thanks for the report. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] relpersistence and temp table
On Fri, Jul 1, 2011 at 10:32 AM, Robert Haas robertmh...@gmail.com wrote: On Fri, Jul 1, 2011 at 8:06 AM, Amit Khandekar amit.khande...@enterprisedb.com wrote: In 9.1, if a table is created using an explicit pg_temp qualification, the pg_class.relpersistence is marked 'p', not 't'. That's a bug. Thanks for the report. OK, so I think the problem here is that, in 9.0, it was possible to figure out what value relistemp should take at a very late date, because it was entirely a function of the schema name. A temporary schema implies relistemp = true, while a non-temporary schema implies relistemp = false. However, in 9.1, that clearly won't do, since unlogged and permanent tables can share the same schema. Moreover, by the time we get as far as RelationBuildLocalRelation(), we've already made lots of other decisions based on relpersistence, so it seems that we need to make this correct as early as possible. It's not feasible to do that in the parser, because the creation namespace could also come from search_path: SET search_path = pg_temp; CREATE TABLE foo (a int); So it seems we can't fix this any earlier than RangeVarGetCreationNamespace(). In the attached patch, I took basically that approach, but created a new function RangeVarAdjustRelationPersistence() that does the actual adjusting (since de-constifying RangeVarGetCreationNamespace() didn't seem smart), plus adds a bunch of additional sanity-checking that I previously overlooked. Namely, it forbids: - creating unlogged tables in temporary schemas - creating relations in temporary schemas of other sessions On the other hand, it does allow CREATE TEMP TABLE pg_temp.foo(a int), which was somewhat pointlessly forbidden by previous releases. In short, the code now checks directly what it used to check by inference: that you're not creating a temporary table in a permanent schema, or the other way around. I also rearranged a few other bits of code to make sure that the appropriate fixups happen BEFORE we enforce the condition that temporary tables mustn't be created in security-restricted contexts. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company fix-relpersistence-early.patch Description: Binary data -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
