Thread: Removal of temp tables
Here is a patch that allows people to delete the pg_temp* tables used as
temp tables. They are left around after a backend crash and the only
way previously to remove them was to start postgres with the -O override
option.
I am wondering if pg_temp tables should even be seen as system tables by
IsSystemRelationName(). We have to call them pg_ so user applications
don't display them, but other than that they aren't like system tables.
Comments?
There are no tests to see if the table is actually in use. I can add
them if people want it.
---------------------------------------------------------------------------
test=> CREATE TEMP TABLE test (x int);
CREATE
test=> \dS
List of relations
Name | Type | Owner
----------------+---------+----------
...
pg_temp.6682.0 | table | postgres
...
test=> DROP TABLE "pg_temp.6682.0";
DROP
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Index: src/backend/catalog/aclchk.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/catalog/aclchk.c,v
retrieving revision 1.50
diff -c -r1.50 aclchk.c
*** src/backend/catalog/aclchk.c 2001/06/09 23:21:54 1.50
--- src/backend/catalog/aclchk.c 2001/06/14 04:06:32
***************
*** 32,37 ****
--- 32,38 ----
#include "parser/parse_func.h"
#include "utils/acl.h"
#include "utils/syscache.h"
+ #include "utils/temprel.h"
static int32 aclcheck(Acl *acl, AclId id, AclIdType idtype, AclMode mode);
***************
*** 437,443 ****
*/
if ((mode & (ACL_INSERT | ACL_UPDATE | ACL_DELETE)) &&
!allowSystemTableMods && IsSystemRelationName(relname) &&
! strncmp(relname, "pg_temp.", strlen("pg_temp.")) != 0 &&
!((Form_pg_shadow) GETSTRUCT(tuple))->usecatupd)
{
#ifdef ACLDEBUG
--- 438,444 ----
*/
if ((mode & (ACL_INSERT | ACL_UPDATE | ACL_DELETE)) &&
!allowSystemTableMods && IsSystemRelationName(relname) &&
! !is_temp_relname(relname) &&
!((Form_pg_shadow) GETSTRUCT(tuple))->usecatupd)
{
#ifdef ACLDEBUG
Index: src/backend/catalog/heap.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/catalog/heap.c,v
retrieving revision 1.167
diff -c -r1.167 heap.c
*** src/backend/catalog/heap.c 2001/06/12 05:55:49 1.167
--- src/backend/catalog/heap.c 2001/06/14 04:06:38
***************
*** 281,288 ****
* replace relname of caller with a unique name for a temp
* relation
*/
! snprintf(relname, NAMEDATALEN, "pg_temp.%d.%u",
! (int) MyProcPid, uniqueId++);
}
/*
--- 281,288 ----
* replace relname of caller with a unique name for a temp
* relation
*/
! snprintf(relname, NAMEDATALEN, "%s.%d.%u",
! PG_TEMP_REL_PREFIX, (int) MyProcPid, uniqueId++);
}
/*
***************
*** 874,910 ****
}
- /* ----------------------------------------------------------------
- * heap_drop_with_catalog - removes all record of named relation from catalogs
- *
- * 1) open relation, check for existence, etc.
- * 2) remove inheritance information
- * 3) remove indexes
- * 4) remove pg_class tuple
- * 5) remove pg_attribute tuples and related descriptions
- * 6) remove pg_description tuples
- * 7) remove pg_type tuples
- * 8) RemoveConstraints ()
- * 9) unlink relation
- *
- * old comments
- * Except for vital relations, removes relation from
- * relation catalog, and related attributes from
- * attribute catalog (needed?). (Anything else?)
- *
- * get proper relation from relation catalog (if not arg)
- * scan attribute catalog deleting attributes of reldesc
- * (necessary?)
- * delete relation from relation catalog
- * (How are the tuples of the relation discarded?)
- *
- * XXX Must fix to work with indexes.
- * There may be a better order for doing things.
- * Problems with destroying a deleted database--cannot create
- * a struct reldesc without having an open file descriptor.
- * ----------------------------------------------------------------
- */
-
/* --------------------------------
* RelationRemoveInheritance
*
--- 874,879 ----
***************
*** 1334,1343 ****
heap_close(pg_type_desc, RowExclusiveLock);
}
! /* --------------------------------
! * heap_drop_with_catalog
*
! * --------------------------------
*/
void
heap_drop_with_catalog(const char *relname,
--- 1303,1337 ----
heap_close(pg_type_desc, RowExclusiveLock);
}
! /* ----------------------------------------------------------------
! * heap_drop_with_catalog - removes all record of named relation from catalogs
*
! * 1) open relation, check for existence, etc.
! * 2) remove inheritance information
! * 3) remove indexes
! * 4) remove pg_class tuple
! * 5) remove pg_attribute tuples and related descriptions
! * 6) remove pg_description tuples
! * 7) remove pg_type tuples
! * 8) RemoveConstraints ()
! * 9) unlink relation
! *
! * old comments
! * Except for vital relations, removes relation from
! * relation catalog, and related attributes from
! * attribute catalog (needed?). (Anything else?)
! *
! * get proper relation from relation catalog (if not arg)
! * scan attribute catalog deleting attributes of reldesc
! * (necessary?)
! * delete relation from relation catalog
! * (How are the tuples of the relation discarded?)
! *
! * XXX Must fix to work with indexes.
! * There may be a better order for doing things.
! * Problems with destroying a deleted database--cannot create
! * a struct reldesc without having an open file descriptor.
! * ----------------------------------------------------------------
*/
void
heap_drop_with_catalog(const char *relname,
***************
*** 1360,1367 ****
* prevent deletion of system relations
*/
/* allow temp of pg_class? Guess so. */
! if (!istemp && !allow_system_table_mods &&
! IsSystemRelationName(RelationGetRelationName(rel)))
elog(ERROR, "System relation \"%s\" may not be dropped",
RelationGetRelationName(rel));
--- 1354,1363 ----
* prevent deletion of system relations
*/
/* allow temp of pg_class? Guess so. */
! if (!istemp &&
! !allow_system_table_mods &&
! IsSystemRelationName(RelationGetRelationName(rel)) &&
! !is_temp_relname(RelationGetRelationName(rel)))
elog(ERROR, "System relation \"%s\" may not be dropped",
RelationGetRelationName(rel));
Index: src/backend/commands/vacuum.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/commands/vacuum.c,v
retrieving revision 1.196
diff -c -r1.196 vacuum.c
*** src/backend/commands/vacuum.c 2001/06/13 21:44:40 1.196
--- src/backend/commands/vacuum.c 2001/06/14 04:06:40
***************
*** 491,497 ****
vacuum_pages.num_pages = fraged_pages.num_pages = 0;
scan_heap(vacrelstats, onerel, &vacuum_pages, &fraged_pages);
if (IsIgnoringSystemIndexes() &&
! IsSystemRelationName(RelationGetRelationName(onerel)))
reindex = true;
/* Now open indices */
--- 491,498 ----
vacuum_pages.num_pages = fraged_pages.num_pages = 0;
scan_heap(vacrelstats, onerel, &vacuum_pages, &fraged_pages);
if (IsIgnoringSystemIndexes() &&
! IsSystemRelationName(RelationGetRelationName(onerel)) &&
! !is_temp_relname(RelationGetRelationName(onerel)))
reindex = true;
/* Now open indices */
Index: src/backend/tcop/utility.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/tcop/utility.c,v
retrieving revision 1.113
diff -c -r1.113 utility.c
*** src/backend/tcop/utility.c 2001/06/09 23:21:54 1.113
--- src/backend/tcop/utility.c 2001/06/14 04:06:44
***************
*** 46,51 ****
--- 46,52 ----
#include "utils/acl.h"
#include "utils/ps_status.h"
#include "utils/syscache.h"
+ #include "utils/temprel.h"
#include "access/xlog.h"
/*
***************
*** 120,126 ****
elog(ERROR, "you do not own %s \"%s\"",
rentry->name, name);
! if (!allowSystemTableMods && IsSystemRelationName(name))
elog(ERROR, "%s \"%s\" is a system %s",
rentry->name, name, rentry->name);
--- 121,128 ----
elog(ERROR, "you do not own %s \"%s\"",
rentry->name, name);
! if (!allowSystemTableMods && IsSystemRelationName(name) &&
! !is_temp_relname(name))
elog(ERROR, "%s \"%s\" is a system %s",
rentry->name, name, rentry->name);
Index: src/include/utils/temprel.h
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/include/utils/temprel.h,v
retrieving revision 1.15
diff -c -r1.15 temprel.h
*** src/include/utils/temprel.h 2001/03/22 04:01:14 1.15
--- src/include/utils/temprel.h 2001/06/14 04:06:47
***************
*** 16,21 ****
--- 16,26 ----
#include "access/htup.h"
+ #define PG_TEMP_REL_PREFIX "pg_temp"
+
+ #define is_temp_relname(relname) \
+ (!strncmp(relname, PG_TEMP_REL_PREFIX, strlen(PG_TEMP_REL_PREFIX)))
+
extern void create_temp_relation(const char *relname,
HeapTuple pg_class_tuple);
extern void remove_temp_rel_by_relid(Oid relid);
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I am wondering if pg_temp tables should even be seen as system tables by
> IsSystemRelationName(). We have to call them pg_ so user applications
> don't display them, but other than that they aren't like system tables.
> Comments?
This oughta be discussed on pghackers, not just -patches. But my
thought is that we need a three-way distinction; at least some of the
IsSystemRelation checks presumably *should* accept temp relnames, else
we'd not have decided to do it that way in the first place.
Another point is that when we implement schemas (= Real Soon Now, I
trust), the whole business of temprels having different physical and
logical relnames will go away anyhow. Temp rels will become plain rels
that live in a temp schema. So it may not be worth adding further
complexity to support the present approach. We'll just have to rip
it out again ... better to expend the work on making schemas.
regards, tom lane
> Bruce Momjian <pgman@candle.pha.pa.us> writes:
> > I am wondering if pg_temp tables should even be seen as system tables by
> > IsSystemRelationName(). We have to call them pg_ so user applications
> > don't display them, but other than that they aren't like system tables.
> > Comments?
>
> This oughta be discussed on pghackers, not just -patches. But my
> thought is that we need a three-way distinction; at least some of the
> IsSystemRelation checks presumably *should* accept temp relnames, else
> we'd not have decided to do it that way in the first place.
>
> Another point is that when we implement schemas (= Real Soon Now, I
> trust), the whole business of temprels having different physical and
> logical relnames will go away anyhow. Temp rels will become plain rels
> that live in a temp schema. So it may not be worth adding further
> complexity to support the present approach. We'll just have to rip
> it out again ... better to expend the work on making schemas.
Here is an updated patch that uses underscores in temp table names so
the DROP doesn't have to quote the table name:
pg_temp_7199_0
I will apply this in two days if there are no other comments.
--
Bruce Momjian | http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 853-3000
+ If your life is a hard drive, | 830 Blythe Avenue
+ Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Index: src/backend/catalog/aclchk.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/catalog/aclchk.c,v
retrieving revision 1.50
diff -c -r1.50 aclchk.c
*** src/backend/catalog/aclchk.c 2001/06/09 23:21:54 1.50
--- src/backend/catalog/aclchk.c 2001/06/14 16:33:47
***************
*** 32,37 ****
--- 32,38 ----
#include "parser/parse_func.h"
#include "utils/acl.h"
#include "utils/syscache.h"
+ #include "utils/temprel.h"
static int32 aclcheck(Acl *acl, AclId id, AclIdType idtype, AclMode mode);
***************
*** 437,443 ****
*/
if ((mode & (ACL_INSERT | ACL_UPDATE | ACL_DELETE)) &&
!allowSystemTableMods && IsSystemRelationName(relname) &&
! strncmp(relname, "pg_temp.", strlen("pg_temp.")) != 0 &&
!((Form_pg_shadow) GETSTRUCT(tuple))->usecatupd)
{
#ifdef ACLDEBUG
--- 438,444 ----
*/
if ((mode & (ACL_INSERT | ACL_UPDATE | ACL_DELETE)) &&
!allowSystemTableMods && IsSystemRelationName(relname) &&
! !is_temp_relname(relname) &&
!((Form_pg_shadow) GETSTRUCT(tuple))->usecatupd)
{
#ifdef ACLDEBUG
Index: src/backend/catalog/heap.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/catalog/heap.c,v
retrieving revision 1.167
diff -c -r1.167 heap.c
*** src/backend/catalog/heap.c 2001/06/12 05:55:49 1.167
--- src/backend/catalog/heap.c 2001/06/14 16:33:51
***************
*** 281,288 ****
* replace relname of caller with a unique name for a temp
* relation
*/
! snprintf(relname, NAMEDATALEN, "pg_temp.%d.%u",
! (int) MyProcPid, uniqueId++);
}
/*
--- 281,288 ----
* replace relname of caller with a unique name for a temp
* relation
*/
! snprintf(relname, NAMEDATALEN, "%s_%d_%u",
! PG_TEMP_REL_PREFIX, (int) MyProcPid, uniqueId++);
}
/*
***************
*** 874,910 ****
}
- /* ----------------------------------------------------------------
- * heap_drop_with_catalog - removes all record of named relation from catalogs
- *
- * 1) open relation, check for existence, etc.
- * 2) remove inheritance information
- * 3) remove indexes
- * 4) remove pg_class tuple
- * 5) remove pg_attribute tuples and related descriptions
- * 6) remove pg_description tuples
- * 7) remove pg_type tuples
- * 8) RemoveConstraints ()
- * 9) unlink relation
- *
- * old comments
- * Except for vital relations, removes relation from
- * relation catalog, and related attributes from
- * attribute catalog (needed?). (Anything else?)
- *
- * get proper relation from relation catalog (if not arg)
- * scan attribute catalog deleting attributes of reldesc
- * (necessary?)
- * delete relation from relation catalog
- * (How are the tuples of the relation discarded?)
- *
- * XXX Must fix to work with indexes.
- * There may be a better order for doing things.
- * Problems with destroying a deleted database--cannot create
- * a struct reldesc without having an open file descriptor.
- * ----------------------------------------------------------------
- */
-
/* --------------------------------
* RelationRemoveInheritance
*
--- 874,879 ----
***************
*** 1334,1343 ****
heap_close(pg_type_desc, RowExclusiveLock);
}
! /* --------------------------------
! * heap_drop_with_catalog
*
! * --------------------------------
*/
void
heap_drop_with_catalog(const char *relname,
--- 1303,1337 ----
heap_close(pg_type_desc, RowExclusiveLock);
}
! /* ----------------------------------------------------------------
! * heap_drop_with_catalog - removes all record of named relation from catalogs
*
! * 1) open relation, check for existence, etc.
! * 2) remove inheritance information
! * 3) remove indexes
! * 4) remove pg_class tuple
! * 5) remove pg_attribute tuples and related descriptions
! * 6) remove pg_description tuples
! * 7) remove pg_type tuples
! * 8) RemoveConstraints ()
! * 9) unlink relation
! *
! * old comments
! * Except for vital relations, removes relation from
! * relation catalog, and related attributes from
! * attribute catalog (needed?). (Anything else?)
! *
! * get proper relation from relation catalog (if not arg)
! * scan attribute catalog deleting attributes of reldesc
! * (necessary?)
! * delete relation from relation catalog
! * (How are the tuples of the relation discarded?)
! *
! * XXX Must fix to work with indexes.
! * There may be a better order for doing things.
! * Problems with destroying a deleted database--cannot create
! * a struct reldesc without having an open file descriptor.
! * ----------------------------------------------------------------
*/
void
heap_drop_with_catalog(const char *relname,
***************
*** 1360,1367 ****
* prevent deletion of system relations
*/
/* allow temp of pg_class? Guess so. */
! if (!istemp && !allow_system_table_mods &&
! IsSystemRelationName(RelationGetRelationName(rel)))
elog(ERROR, "System relation \"%s\" may not be dropped",
RelationGetRelationName(rel));
--- 1354,1363 ----
* prevent deletion of system relations
*/
/* allow temp of pg_class? Guess so. */
! if (!istemp &&
! !allow_system_table_mods &&
! IsSystemRelationName(RelationGetRelationName(rel)) &&
! !is_temp_relname(RelationGetRelationName(rel)))
elog(ERROR, "System relation \"%s\" may not be dropped",
RelationGetRelationName(rel));
Index: src/backend/commands/vacuum.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/commands/vacuum.c,v
retrieving revision 1.196
diff -c -r1.196 vacuum.c
*** src/backend/commands/vacuum.c 2001/06/13 21:44:40 1.196
--- src/backend/commands/vacuum.c 2001/06/14 16:33:53
***************
*** 491,497 ****
vacuum_pages.num_pages = fraged_pages.num_pages = 0;
scan_heap(vacrelstats, onerel, &vacuum_pages, &fraged_pages);
if (IsIgnoringSystemIndexes() &&
! IsSystemRelationName(RelationGetRelationName(onerel)))
reindex = true;
/* Now open indices */
--- 491,498 ----
vacuum_pages.num_pages = fraged_pages.num_pages = 0;
scan_heap(vacrelstats, onerel, &vacuum_pages, &fraged_pages);
if (IsIgnoringSystemIndexes() &&
! IsSystemRelationName(RelationGetRelationName(onerel)) &&
! !is_temp_relname(RelationGetRelationName(onerel)))
reindex = true;
/* Now open indices */
Index: src/backend/storage/file/fd.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/storage/file/fd.c,v
retrieving revision 1.81
diff -c -r1.81 fd.c
*** src/backend/storage/file/fd.c 2001/06/11 04:12:29 1.81
--- src/backend/storage/file/fd.c 2001/06/14 16:34:02
***************
*** 756,762 ****
* transaction and database instance.
*/
snprintf(tempfilepath, sizeof(tempfilepath),
! "%s/%s%d.%ld", PG_TEMP_FILES_DIR, PG_TEMP_FILE_PREFIX,
MyProcPid, tempFileCounter++);
/*
--- 756,762 ----
* transaction and database instance.
*/
snprintf(tempfilepath, sizeof(tempfilepath),
! "%s/%s%d_%ld", PG_TEMP_FILES_DIR, PG_TEMP_FILE_PREFIX,
MyProcPid, tempFileCounter++);
/*
Index: src/backend/tcop/utility.c
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/tcop/utility.c,v
retrieving revision 1.113
diff -c -r1.113 utility.c
*** src/backend/tcop/utility.c 2001/06/09 23:21:54 1.113
--- src/backend/tcop/utility.c 2001/06/14 16:34:02
***************
*** 46,51 ****
--- 46,52 ----
#include "utils/acl.h"
#include "utils/ps_status.h"
#include "utils/syscache.h"
+ #include "utils/temprel.h"
#include "access/xlog.h"
/*
***************
*** 120,126 ****
elog(ERROR, "you do not own %s \"%s\"",
rentry->name, name);
! if (!allowSystemTableMods && IsSystemRelationName(name))
elog(ERROR, "%s \"%s\" is a system %s",
rentry->name, name, rentry->name);
--- 121,128 ----
elog(ERROR, "you do not own %s \"%s\"",
rentry->name, name);
! if (!allowSystemTableMods && IsSystemRelationName(name) &&
! !is_temp_relname(name))
elog(ERROR, "%s \"%s\" is a system %s",
rentry->name, name, rentry->name);
Index: src/include/utils/temprel.h
===================================================================
RCS file: /home/projects/pgsql/cvsroot/pgsql/src/include/utils/temprel.h,v
retrieving revision 1.15
diff -c -r1.15 temprel.h
*** src/include/utils/temprel.h 2001/03/22 04:01:14 1.15
--- src/include/utils/temprel.h 2001/06/14 16:34:03
***************
*** 16,21 ****
--- 16,26 ----
#include "access/htup.h"
+ #define PG_TEMP_REL_PREFIX "pg_temp"
+
+ #define is_temp_relname(relname) \
+ (!strncmp(relname, PG_TEMP_REL_PREFIX, strlen(PG_TEMP_REL_PREFIX)))
+
extern void create_temp_relation(const char *relname,
HeapTuple pg_class_tuple);
extern void remove_temp_rel_by_relid(Oid relid);
Bruce Momjian <pgman@candle.pha.pa.us> writes: > Here is an updated patch that uses underscores in temp table names so > the DROP doesn't have to quote the table name: That seems like a reasonable idea, but don't do it to temp file names, ie, drop this part of the diff: > *** src/backend/storage/file/fd.c 2001/06/11 04:12:29 1.81 > --- src/backend/storage/file/fd.c 2001/06/14 16:34:02 > *************** > *** 756,762 **** > * transaction and database instance. > */ > snprintf(tempfilepath, sizeof(tempfilepath), > ! "%s/%s%d.%ld", PG_TEMP_FILES_DIR, PG_TEMP_FILE_PREFIX, > MyProcPid, tempFileCounter++); > /* > --- 756,762 ---- > * transaction and database instance. > */ > snprintf(tempfilepath, sizeof(tempfilepath), > ! "%s/%s%d_%ld", PG_TEMP_FILES_DIR, PG_TEMP_FILE_PREFIX, > MyProcPid, tempFileCounter++); > /* There's no reason to spell temp file names as if they were rel names, and probably it's best not to make them look the same. Also, an item I've ranted about before: > + #define is_temp_relname(relname) \ > + (!strncmp(relname, PG_TEMP_REL_PREFIX, strlen(PG_TEMP_REL_PREFIX))) It's bad style to treat the result of strcmp or strncmp as though it were a boolean, cf http://fts.postgresql.org/db/mw/msg.html?mid=68294 Write (strncmp(...) == 0) instead. Otherwise the patch seems reasonable, although I wonder what your motivation was for choosing these particular IsSystemRelationName calls to tweak. It looks like you did more than the minimum needed to allow a DROP TABLE; why these extra ones and not others? (Not that I'm encouraging you to go around and hit every IsSystemRelationName call. If you did, that'll just be more changes that I suspect we'll have to remove again in the long run. I'm just curious why you touched, for example, VACUUM.) regards, tom lane
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > > Here is an updated patch that uses underscores in temp table names so > > the DROP doesn't have to quote the table name: > > That seems like a reasonable idea, but don't do it to temp file > names, ie, drop this part of the diff: > > > *** src/backend/storage/file/fd.c 2001/06/11 04:12:29 1.81 > > --- src/backend/storage/file/fd.c 2001/06/14 16:34:02 > > *************** > > *** 756,762 **** > > * transaction and database instance. > > */ > > snprintf(tempfilepath, sizeof(tempfilepath), > > ! "%s/%s%d.%ld", PG_TEMP_FILES_DIR, PG_TEMP_FILE_PREFIX, > > MyProcPid, tempFileCounter++); > > > /* > > --- 756,762 ---- > > * transaction and database instance. > > */ > > snprintf(tempfilepath, sizeof(tempfilepath), > > ! "%s/%s%d_%ld", PG_TEMP_FILES_DIR, PG_TEMP_FILE_PREFIX, > > MyProcPid, tempFileCounter++); > > > /* > > There's no reason to spell temp file names as if they were rel names, > and probably it's best not to make them look the same. I was wondering that. The old vacuum file detection patch had the sort files going into /pg_sorttemp and files called pid_. Your changes made it pg_tempfile directory and pg_temp file names. I like the older names that made them clear they were _not_ temp tables. Seemed you wanted them to have similar names for reasons I couldn't figure. I don't care if the sort files have dots so I will remove that part of the patch, but I think we should consider making the sort files more different than they are now --- dots vs. underscores. > Also, an item I've ranted about before: > > > + #define is_temp_relname(relname) \ > > + (!strncmp(relname, PG_TEMP_REL_PREFIX, strlen(PG_TEMP_REL_PREFIX))) > > It's bad style to treat the result of strcmp or strncmp as though it > were a boolean, cf > http://fts.postgresql.org/db/mw/msg.html?mid=68294 > Write (strncmp(...) == 0) instead. OK, changed. > Otherwise the patch seems reasonable, although I wonder what your > motivation was for choosing these particular IsSystemRelationName calls > to tweak. It looks like you did more than the minimum needed to allow > a DROP TABLE; why these extra ones and not others? (Not that I'm > encouraging you to go around and hit every IsSystemRelationName call. > If you did, that'll just be more changes that I suspect we'll have to > remove again in the long run. I'm just curious why you touched, for > example, VACUUM.) I removed the vacuum part. I added it because it looked particularly bad to do REINDEX on temp tables but I have no reason to know that for sure. Patch attached. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> I was wondering that. The old vacuum file detection patch had the sort
> files going into /pg_sorttemp and files called pid_. Your changes made
> it pg_tempfile directory and pg_temp file names. I like the older names
> that made them clear they were _not_ temp tables. Seemed you wanted
> them to have similar names for reasons I couldn't figure.
I'm not wedded to those names; if you have a better idea, let's hear it.
I changed it because I didn't like the use of the word "sort"; temp
files are not used only for sorts, but for several other things, and so
I wanted to see them called temp files not sorttemp files. But if you
want to change them so that they look even less like the logical names
of temp relations, that's OK with me.
regards, tom lane
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > > I was wondering that. The old vacuum file detection patch had the sort > > files going into /pg_sorttemp and files called pid_. Your changes made > > it pg_tempfile directory and pg_temp file names. I like the older names > > that made them clear they were _not_ temp tables. Seemed you wanted > > them to have similar names for reasons I couldn't figure. > > I'm not wedded to those names; if you have a better idea, let's hear it. > I changed it because I didn't like the use of the word "sort"; temp > files are not used only for sorts, but for several other things, and so > I wanted to see them called temp files not sorttemp files. But if you > want to change them so that they look even less like the logical names > of temp relations, that's OK with me. OK, I will do that. What else do we do with them except sorts? Seems pid_ was a good file name because they are always based on pid in storage/file/fd.c. The directory could be called simply 'tempfile' with no pg_. How is that? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
> What else do we do with them except sorts?
Hash joins. Materialize nodes. Not to mention that sorting is used for
things that aren't obviously sorts (SELECT count(distinct foo), for
example).
> Seems
> pid_ was a good file name because they are always based on pid in
> storage/file/fd.c. The directory could be called simply 'tempfile' with
> no pg_. How is that?
You had that to begin with, and I changed it because I thought it was a
bad idea. The directory name and file name should both make perfectly
clear that the files are temp files belonging to Postgres. For example,
it would be unsafe to make pg_tempfiles be a symlink pointing to a temp
directory shared with other apps if there was any risk of temp file name
collisions. (Not sure you'd do that anyway, because of security issues,
but let's not foreclose it with a poor choice of file names.) A purely
numeric file name for temp files is a particularly bad idea because it
looks too much like our numeric names for table data files. Don't
eliminate a hypothetical confusion factor between relnames and filenames
(which are never seen in the same context anyway) by introducing one
between filenames and other filenames.
If you don't like pg_temp here, maybe post_temp? pgsql_temp?
regards, tom lane
> You had that to begin with, and I changed it because I thought it was a > bad idea. The directory name and file name should both make perfectly > clear that the files are temp files belonging to Postgres. For example, > it would be unsafe to make pg_tempfiles be a symlink pointing to a temp > directory shared with other apps if there was any risk of temp file name > collisions. (Not sure you'd do that anyway, because of security issues, > but let's not foreclose it with a poor choice of file names.) A purely > numeric file name for temp files is a particularly bad idea because it > looks too much like our numeric names for table data files. Don't > eliminate a hypothetical confusion factor between relnames and filenames > (which are never seen in the same context anyway) by introducing one > between filenames and other filenames. > > If you don't like pg_temp here, maybe post_temp? pgsql_temp? What if I call the directory tmp or pgsql_tmp and the files pgsql_pid_#.#? -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
Bruce Momjian <pgman@candle.pha.pa.us> writes:
>> If you don't like pg_temp here, maybe post_temp? pgsql_temp?
> What if I call the directory tmp or pgsql_tmp and the files
> pgsql_pid_#.#?
Why are you so eager not to call temp files "temp"? If you want to
spell it "pgsql_tmp" or "pg_temporary" or something different from
"pg_temp", that's fine, but I really think the files ought to clearly
label themselves as temporary. "pid" is not a clear label. With a
name like that, people might even think they are lock files.
regards, tom lane
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> If you don't like pg_temp here, maybe post_temp? pgsql_temp? > > > What if I call the directory tmp or pgsql_tmp and the files > > pgsql_pid_#.#? > > Why are you so eager not to call temp files "temp"? If you want to > spell it "pgsql_tmp" or "pg_temporary" or something different from > "pg_temp", that's fine, but I really think the files ought to clearly > label themselves as temporary. "pid" is not a clear label. With a > name like that, people might even think they are lock files. I wanted to call them pid so people knew what the file number meant. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > >> If you don't like pg_temp here, maybe post_temp? pgsql_temp? > > > What if I call the directory tmp or pgsql_tmp and the files > > pgsql_pid_#.#? > > Why are you so eager not to call temp files "temp"? If you want to > spell it "pgsql_tmp" or "pg_temporary" or something different from > "pg_temp", that's fine, but I really think the files ought to clearly > label themselves as temporary. "pid" is not a clear label. With a > name like that, people might even think they are lock files. OK, here is the new version with directories and files called pgsql_tmp. That doesn't look like it holds temp tables. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026 Index: src/backend/catalog/aclchk.c =================================================================== RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/catalog/aclchk.c,v retrieving revision 1.50 diff -c -r1.50 aclchk.c *** src/backend/catalog/aclchk.c 2001/06/09 23:21:54 1.50 --- src/backend/catalog/aclchk.c 2001/06/15 00:01:48 *************** *** 32,37 **** --- 32,38 ---- #include "parser/parse_func.h" #include "utils/acl.h" #include "utils/syscache.h" + #include "utils/temprel.h" static int32 aclcheck(Acl *acl, AclId id, AclIdType idtype, AclMode mode); *************** *** 437,443 **** */ if ((mode & (ACL_INSERT | ACL_UPDATE | ACL_DELETE)) && !allowSystemTableMods && IsSystemRelationName(relname) && ! strncmp(relname, "pg_temp.", strlen("pg_temp.")) != 0 && !((Form_pg_shadow) GETSTRUCT(tuple))->usecatupd) { #ifdef ACLDEBUG --- 438,444 ---- */ if ((mode & (ACL_INSERT | ACL_UPDATE | ACL_DELETE)) && !allowSystemTableMods && IsSystemRelationName(relname) && ! !is_temp_relname(relname) && !((Form_pg_shadow) GETSTRUCT(tuple))->usecatupd) { #ifdef ACLDEBUG Index: src/backend/catalog/heap.c =================================================================== RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/catalog/heap.c,v retrieving revision 1.167 diff -c -r1.167 heap.c *** src/backend/catalog/heap.c 2001/06/12 05:55:49 1.167 --- src/backend/catalog/heap.c 2001/06/15 00:01:49 *************** *** 281,288 **** * replace relname of caller with a unique name for a temp * relation */ ! snprintf(relname, NAMEDATALEN, "pg_temp.%d.%u", ! (int) MyProcPid, uniqueId++); } /* --- 281,288 ---- * replace relname of caller with a unique name for a temp * relation */ ! snprintf(relname, NAMEDATALEN, "%s_%d_%u", ! PG_TEMP_REL_PREFIX, (int) MyProcPid, uniqueId++); } /* *************** *** 874,910 **** } - /* ---------------------------------------------------------------- - * heap_drop_with_catalog - removes all record of named relation from catalogs - * - * 1) open relation, check for existence, etc. - * 2) remove inheritance information - * 3) remove indexes - * 4) remove pg_class tuple - * 5) remove pg_attribute tuples and related descriptions - * 6) remove pg_description tuples - * 7) remove pg_type tuples - * 8) RemoveConstraints () - * 9) unlink relation - * - * old comments - * Except for vital relations, removes relation from - * relation catalog, and related attributes from - * attribute catalog (needed?). (Anything else?) - * - * get proper relation from relation catalog (if not arg) - * scan attribute catalog deleting attributes of reldesc - * (necessary?) - * delete relation from relation catalog - * (How are the tuples of the relation discarded?) - * - * XXX Must fix to work with indexes. - * There may be a better order for doing things. - * Problems with destroying a deleted database--cannot create - * a struct reldesc without having an open file descriptor. - * ---------------------------------------------------------------- - */ - /* -------------------------------- * RelationRemoveInheritance * --- 874,879 ---- *************** *** 1334,1343 **** heap_close(pg_type_desc, RowExclusiveLock); } ! /* -------------------------------- ! * heap_drop_with_catalog * ! * -------------------------------- */ void heap_drop_with_catalog(const char *relname, --- 1303,1337 ---- heap_close(pg_type_desc, RowExclusiveLock); } ! /* ---------------------------------------------------------------- ! * heap_drop_with_catalog - removes all record of named relation from catalogs * ! * 1) open relation, check for existence, etc. ! * 2) remove inheritance information ! * 3) remove indexes ! * 4) remove pg_class tuple ! * 5) remove pg_attribute tuples and related descriptions ! * 6) remove pg_description tuples ! * 7) remove pg_type tuples ! * 8) RemoveConstraints () ! * 9) unlink relation ! * ! * old comments ! * Except for vital relations, removes relation from ! * relation catalog, and related attributes from ! * attribute catalog (needed?). (Anything else?) ! * ! * get proper relation from relation catalog (if not arg) ! * scan attribute catalog deleting attributes of reldesc ! * (necessary?) ! * delete relation from relation catalog ! * (How are the tuples of the relation discarded?) ! * ! * XXX Must fix to work with indexes. ! * There may be a better order for doing things. ! * Problems with destroying a deleted database--cannot create ! * a struct reldesc without having an open file descriptor. ! * ---------------------------------------------------------------- */ void heap_drop_with_catalog(const char *relname, *************** *** 1360,1367 **** * prevent deletion of system relations */ /* allow temp of pg_class? Guess so. */ ! if (!istemp && !allow_system_table_mods && ! IsSystemRelationName(RelationGetRelationName(rel))) elog(ERROR, "System relation \"%s\" may not be dropped", RelationGetRelationName(rel)); --- 1354,1363 ---- * prevent deletion of system relations */ /* allow temp of pg_class? Guess so. */ ! if (!istemp && ! !allow_system_table_mods && ! IsSystemRelationName(RelationGetRelationName(rel)) && ! !is_temp_relname(RelationGetRelationName(rel))) elog(ERROR, "System relation \"%s\" may not be dropped", RelationGetRelationName(rel)); Index: src/backend/storage/file/fd.c =================================================================== RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/storage/file/fd.c,v retrieving revision 1.81 diff -c -r1.81 fd.c *** src/backend/storage/file/fd.c 2001/06/11 04:12:29 1.81 --- src/backend/storage/file/fd.c 2001/06/15 00:01:52 *************** *** 54,61 **** /* Filename components for OpenTemporaryFile */ ! #define PG_TEMP_FILES_DIR "pg_tempfiles" ! #define PG_TEMP_FILE_PREFIX "pg_temp" /* --- 54,61 ---- /* Filename components for OpenTemporaryFile */ ! #define PG_TEMP_FILES_DIR "pgsql_tmp" ! #define PG_TEMP_FILE_PREFIX "pgsql_tmp" /* Index: src/backend/tcop/utility.c =================================================================== RCS file: /home/projects/pgsql/cvsroot/pgsql/src/backend/tcop/utility.c,v retrieving revision 1.113 diff -c -r1.113 utility.c *** src/backend/tcop/utility.c 2001/06/09 23:21:54 1.113 --- src/backend/tcop/utility.c 2001/06/15 00:01:52 *************** *** 46,51 **** --- 46,52 ---- #include "utils/acl.h" #include "utils/ps_status.h" #include "utils/syscache.h" + #include "utils/temprel.h" #include "access/xlog.h" /* *************** *** 120,126 **** elog(ERROR, "you do not own %s \"%s\"", rentry->name, name); ! if (!allowSystemTableMods && IsSystemRelationName(name)) elog(ERROR, "%s \"%s\" is a system %s", rentry->name, name, rentry->name); --- 121,128 ---- elog(ERROR, "you do not own %s \"%s\"", rentry->name, name); ! if (!allowSystemTableMods && IsSystemRelationName(name) && ! !is_temp_relname(name)) elog(ERROR, "%s \"%s\" is a system %s", rentry->name, name, rentry->name); Index: src/include/utils/temprel.h =================================================================== RCS file: /home/projects/pgsql/cvsroot/pgsql/src/include/utils/temprel.h,v retrieving revision 1.15 diff -c -r1.15 temprel.h *** src/include/utils/temprel.h 2001/03/22 04:01:14 1.15 --- src/include/utils/temprel.h 2001/06/15 00:01:53 *************** *** 16,21 **** --- 16,26 ---- #include "access/htup.h" + #define PG_TEMP_REL_PREFIX "pg_temp" + + #define is_temp_relname(relname) \ + (strncmp(relname, PG_TEMP_REL_PREFIX, strlen(PG_TEMP_REL_PREFIX)) == 0) + extern void create_temp_relation(const char *relname, HeapTuple pg_class_tuple); extern void remove_temp_rel_by_relid(Oid relid);
> Bruce Momjian <pgman@candle.pha.pa.us> writes: > > What else do we do with them except sorts? > > Hash joins. Materialize nodes. Not to mention that sorting is used for > things that aren't obviously sorts (SELECT count(distinct foo), for > example). Oh, I didn't know that. OK. > > Seems > > pid_ was a good file name because they are always based on pid in > > storage/file/fd.c. The directory could be called simply 'tempfile' with > > no pg_. How is that? > > You had that to begin with, and I changed it because I thought it was a > bad idea. The directory name and file name should both make perfectly > clear that the files are temp files belonging to Postgres. For example, > it would be unsafe to make pg_tempfiles be a symlink pointing to a temp > directory shared with other apps if there was any risk of temp file name > collisions. (Not sure you'd do that anyway, because of security issues, > but let's not foreclose it with a poor choice of file names.) A purely > numeric file name for temp files is a particularly bad idea because it > looks too much like our numeric names for table data files. Don't > eliminate a hypothetical confusion factor between relnames and filenames > (which are never seen in the same context anyway) by introducing one > between filenames and other filenames. OK, I see, you think it could share a directory with another app. Now I see why you used pg_temp. > > If you don't like pg_temp here, maybe post_temp? pgsql_temp? OK, pgsql_temp works for me. Everytime I see pg_ I think system table. I will use pgsql_temp for directory and file names. -- Bruce Momjian | http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 853-3000 + If your life is a hard drive, | 830 Blythe Avenue + Christ can be your backup. | Drexel Hill, Pennsylvania 19026