Re: relpersistence and temp table

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 "temp"ness 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?

Regards
-Amit Khandekar

On Fri, Jul 1, 2011 at 8:06 AM, Amit Khandekar
<amit(dot)khandekar(at)enterprisedb(dot)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 "temp"ness 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

On Fri, Jul 1, 2011 at 10:32 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jul 1, 2011 at 8:06 AM, Amit Khandekar
> <amit(dot)khandekar(at)enterprisedb(dot)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

On Fri, Jul 1, 2011 at 1:59 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Fri, Jul 1, 2011 at 10:32 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> On Fri, Jul 1, 2011 at 8:06 AM, Amit Khandekar
>> <amit(dot)khandekar(at)enterprisedb(dot)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

Robert Haas wrote:
> On Fri, Jul 1, 2011 at 10:32 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Fri, Jul 1, 2011 at 8:06 AM, Amit Khandekar
> > <amit(dot)khandekar(at)enterprisedb(dot)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 <bruce(at)momjian(dot)us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ It's impossible for everything to be true. +

On Jul 11, 2011, at 8:55 PM, Bruce Momjian <bruce(at)momjian(dot)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