Re: [PATCHES] Added columns to pg_stat_activity
Magnus Hagander wrote: Updated patch attached. A few more comments: - why did you add the client address to PgStat_MsgHdr, rather than PgStat_MsgBestart? It would be nice to avoid sending the client address for each and every stats message, as it shouldn't change over the life of the session. - I think the backend_client_addr and backend_client_port view columns should be named client_addr and client_port, respectively. - Is there a reason you need to expose and then call the private network_in() function, rather than using inet_in(), which is already public? - Is the backend startup time sensitive information? Considering this information is available via ps(1), perhaps it would be better to allow any user to see any backend's startup time, rather than providing a false sense of security. - We return (NULL, -1) for the client address/port if connected via a unix domain socket, and (NULL, NULL) for the address/port if the selecting user isn't a superuser / the owner of that client connection. It seems a little ugly to return NULL for the address in both cases, since the same value is used for two completely different meanings. Not sure of a better convention, though -- just idly complaining :) You needn't respin the patch, as I can make the necessary changes myself -- I'm just looking for comments on the above before applying. -Neil ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Added columns to pg_stat_activity
Updated patch attached. A few more comments: - why did you add the client address to PgStat_MsgHdr, rather than PgStat_MsgBestart? It would be nice to avoid sending the client address for each and every stats message, as it shouldn't change over the life of the session. I guess that's from not reading things carefully enough. I looked for where backend pid was transmitted, because I thought that would be a good place to put it. Which might bring up the question, why are things like the backend pid sent in msghdr and not in bestart? (seems bestart is nothing more than the header, which is why I missed it completely) - I think the backend_client_addr and backend_client_port view columns should be named client_addr and client_port, respectively. Seems reasonable. - Is there a reason you need to expose and then call the private network_in() function, rather than using inet_in(), which is already public? Not really, that part of the code was a copy from client_addr(). - Is the backend startup time sensitive information? Considering this information is available via ps(1), perhaps it would be better to allow any user to see any backend's startup time, rather than providing a false sense of security. Well, ps(1) will show you the client address as well, so the argument can be taken there too. For ps(1) access, you need shell access to the machine. Something at least I would *never* give to people who just do stuff in the database. That said, I'm not sure that the startup time specifically is sensitive, no. No real rason to hide that, but I'm not sure I buy the argument considering since it's available via ps(1) ;-) - We return (NULL, -1) for the client address/port if connected via a unix domain socket, and (NULL, NULL) for the address/port if the selecting user isn't a superuser / the owner of that client connection. It seems a little ugly to return NULL for the address in both cases, since the same value is used for two completely different meanings. Not sure of a better convention, though -- just idly complaining :) I first did it with NULL,NULL, before I realised I could not make just that distinction. I guess in theory we could return 0.0.0.0 or 255.255.255.255 (similar for IPv6 of course, don't know offhand what that would be), btu that seems at least as ugly. Another way would be to have yet another column that would be client address type, but given the options I think the current way is probably the least ugly. You needn't respin the patch, as I can make the necessary changes myself -- I'm just looking for comments on the above before applying. Ok. //Magnus ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] Added columns to pg_stat_activity
Magnus Hagander [EMAIL PROTECTED] writes: I guess that's from not reading things carefully enough. I looked for where backend pid was transmitted, because I thought that would be a good place to put it. Which might bring up the question, why are things like the backend pid sent in msghdr and not in bestart? I think the PID is used as the lookup key to associate messages from the same backend. You need some such key, though BackendId (sinval slot number) is another workable candidate. regards, tom lane ---(end of broadcast)--- TIP 6: Have you searched our list archives? http://archives.postgresql.org
Re: [PATCHES] Added columns to pg_stat_activity
Neil Conway [EMAIL PROTECTED] writes: - Is the backend startup time sensitive information? Considering this information is available via ps(1), perhaps it would be better to allow any user to see any backend's startup time, rather than providing a false sense of security. Remote database users don't have access to ps(1). I think that hiding the startup time is reasonable. - We return (NULL, -1) for the client address/port if connected via a unix domain socket, and (NULL, NULL) for the address/port if the selecting user isn't a superuser / the owner of that client connection. It seems a little ugly to return NULL for the address in both cases, since the same value is used for two completely different meanings. Not sure of a better convention, though -- just idly complaining :) Perhaps 0.0.0.0/0 or some such for the address in the unix domain case? Not sure that this is an improvement over NULL though. Other comments seem on-target to me. regards, tom lane ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [HACKERS] [PATCHES] Patch for database locale settings
Description added to TODO: * Allow locale to be set at database creation Currently locale can only be set during initdb. No global tables have locale-aware columns. However, the database template used during database creation might have locale-aware indexes. The indexes would need to be reindexed to match the new locale. --- Tom Lane wrote: Alvaro Herrera [EMAIL PROTECTED] writes: Maybe it would work if we forced indexes on shared relations to be scanned using a fixed collation. The shared relations only have indexes on name, oid, and integer: select distinct atttypid::regtype from pg_class c join pg_attribute a on c.oid = a.attrelid where relisshared and relkind = 'i'; and name has non-locale-sensitive ordering rules anyway. So that's not the big problem; we could probably get away with decreeing that name will always be that way and that shared relations can't have locale-dependent indexes. The big problem (and the reason why this idea has been shot down in the past) is that CREATE DATABASE can't change the locale from what it is in the template database unless it's prepared to reindex any locale- sensitive indexes in the non-shared part of the template database. Which would be a difficult undertaking seeing that we can't even connect to the copied database until after commit. We could maybe say that we will never have any locale-dependent indexes at all on any system catalog, but what of user-defined tables in template databases? It would simply not work to do something as simple as creating a table with an indexed text column in template1. On the other hand you could argue that people already run the same kind of risk when changing database encoding at CREATE, which is a feature that's been there a long time and hasn't garnered many complaints. Not so much that their indexes will break as that their data will. So perhaps we should be willing to document don't do that. Certainly it would be a lot more useful if both locale and encoding could be set per-database. regards, tom lane ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] Cleaning up unreferenced table files
Maybe we should take a different approach to the problem: 1. Create new file with an extension to mark that it's not yet committed (eg. 1234.notcommitted) 2. ... 3. Take CheckpointStartLock 4. Write commit record to WAL, with list of created files. 5. rename created file (1234.notcommitted - 1234). 6. Release CheckpointStartLock This would guarantee that after successful WAL replay, all files in the data directory with .notcommitted extension can be safely deleted. No need to read pg_database or pg_class. We would take a performance hit because of the additional rename and fsync step. Also, we must somehow make sure that the new file or the directory it's in is fsynced on checkpoint to make sure that the rename is flushed to disk. A variant of the scheme would be to create two files on step 1. One would be the actual relfile (1234) and the other would an empty marker file (1234.notcommitted). That way the smgr code wouldn't have to care it the file is new or not when opening it. - Heikki On Thu, 5 May 2005, Tom Lane wrote: Bruce Momjian pgman@candle.pha.pa.us writes: Applied. Now that I've had a chance to look at it, this patch is thoroughly broken. Problems observed in a quick review: 1. It doesn't work at all for non-default tablespaces: it will claim that every file in such a tablespace is stale. The fact that it does that rather than failing entirely is accidental. It tries to read the database's pg_class in the target tablespace whether it's there or not. Because the system is still in recovery mode, the low-level routines allow the access to the nonexistent pg_class table to pass --- in fact they think they should create the file, so after it runs there's a bogus empty 1259 file in each such tablespace (which of course it complains about, too). The code then proceeds to think that pg_class is empty so of course everything draws a warning. 2. It's not robust against stale subdirectories of a tablespace (ie, subdirs corresponding to a nonexistent database) --- again, it'll try to read a nonexistent pg_class. Then it'll produce a bunch of off-target complaint messages. 3. It's assuming that relfilenode is unique database-wide, when no such assumption is safe. We only have a guarantee that it's unique tablespace-wide. 4. It fails to examine table segment files (such as nnn.1). These should be complained of when the nnn doesn't match any hash entry. 5. It will load every relfilenode value in pg_class into the hashtable whether it's meaningful or not. There should be a check on relkind. 6. I don't think relying on strtol to decide if a filename is entirely numeric is very safe. Note all the extra defenses in pg_atoi against various platform-specific misbehaviors of strtol. Personally I'd use a strspn test instead. 7. There are no checks for readdir failure (compare any other readdir loop in the backend). See also Simon Riggs' complaints that the circumstances under which it's done are pretty randomly selected. (One particular thing that I think is a bad idea is to do this in a standalone backend. Any sort of corruption in any db's pg_class would render it impossible to start up.) To fix the first three problems, and also avoid the performance problem of multiply rescanning a database's pg_class for each of its tablespaces, I would suggest that the hashtable entries be widened to RelFileNode structs (ie, db oid, tablespace oid, relfilenode oid). Then there should be one iteration over pg_database to learn the OIDs and default tablespaces of each database; with that you can read pg_class from its correct location for each database and load all the entries into the hashtable. Then you iterate through the tablespaces looking for stuff not present in the hashtable. You might also want to build a list or hashtable of known database OIDs, so that you can recognize a stale subdirectory immediately and issue a direct complaint about it without even recursing into it. regards, tom lane - Heikki ---(end of broadcast)--- TIP 5: Have you checked our extensive FAQ? http://www.postgresql.org/docs/faq
Re: [PATCHES] Cleaning up unreferenced table files
Heikki Linnakangas [EMAIL PROTECTED] writes: Maybe we should take a different approach to the problem: 1. Create new file with an extension to mark that it's not yet committed (eg. 1234.notcommitted) This is pushing the problem into the wrong place, viz the lowest-level file access routines, which will now all have to know about .notcommitted status. It also creates race conditions --- think about backend A trying to commit file 1234 at about the same time that backend B is trying to flush some dirty buffers belonging to that file. But most importantly, it doesn't handle the file-deletion case. regards, tom lane ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
[PATCHES] Dealing with CLUSTER failures
Christopher Kings-Lynne wrote:
I don't think that's a bug. You may not intend ever to cluster on that
index again, and if you try it will tell you about the problem.
Except it breaks the 'cluster everything' case:
test=# cluster;
ERROR: cannot cluster when index access method does not handle null values
HINT: You may be able to work around this by marking column a NOT NULL.
I looked over this item, originally posted as:
http://archives.postgresql.org/pgsql-hackers/2005-03/msg01055.php
It seems that if you use an index method that doesn't support NULLs, you
can use ALTER to set the column as NOT NULL, then CLUSTER, and then set
it to allow NULLs, and when you CLUSTER all tables, the cluster errors
out on that table.
I thought about removing the cluster bit when you do the alter or
something like that, but it seems too confusing and error-prone to be
sure we get every case.
I think the main problem is that while cluster is a performance-only
feature, we error out if we can't cluster one table, basically treating
it as though it needs transaction semantics. It doesn't.
This patch throws an ERROR of you cluster a specific index that can't be
clustered, but issues only a WARNING if you are clustering all tables.
This allows it to report the failed cluster but keep going. I also
modified the code to print the index name in case of failure, because
without that the user doesn't know the failing index name in a
database-wide cluster failure.
Here is an example:
test= cluster test_gist_idx on test;
ERROR: cannot cluster on index test_gist_idx because access method
does not handle null values
HINT: You may be able to work around this by marking column a NOT
NULL.
test= cluster;
WARNING: cannot cluster on index test_gist_idx because access method
does not handle null values
HINT: You may be able to work around this by marking column a NOT
NULL.
CLUSTER
You can see the ERROR for a specific index, and WARNING for full
database cluster.
--
Bruce Momjian| http://candle.pha.pa.us
pgman@candle.pha.pa.us | (610) 359-1001
+ If your life is a hard drive, | 13 Roberts Road
+ Christ can be your backup.| Newtown Square, Pennsylvania 19073
Index: src/backend/commands/cluster.c
===
RCS file: /cvsroot/pgsql/src/backend/commands/cluster.c,v
retrieving revision 1.137
diff -c -c -r1.137 cluster.c
*** src/backend/commands/cluster.c 6 May 2005 17:24:53 - 1.137
--- src/backend/commands/cluster.c 8 May 2005 02:40:50 -
***
*** 292,298
OldHeap = heap_open(rvtc-tableOid, AccessExclusiveLock);
/* Check index is valid to cluster on */
! check_index_is_clusterable(OldHeap, rvtc-indexOid);
/* rebuild_relation does all the dirty work */
rebuild_relation(OldHeap, rvtc-indexOid);
--- 292,303
OldHeap = heap_open(rvtc-tableOid, AccessExclusiveLock);
/* Check index is valid to cluster on */
! if (!check_index_is_clusterable(OldHeap, rvtc-indexOid,
! recheck ? WARNING : ERROR))
! {
! heap_close(OldHeap, NoLock);
! return;
! }
/* rebuild_relation does all the dirty work */
rebuild_relation(OldHeap, rvtc-indexOid);
***
*** 308,315
* redundant, but it seems best to grab it anyway to ensure the index
* definition can't change under us.
*/
! void
! check_index_is_clusterable(Relation OldHeap, Oid indexOid)
{
RelationOldIndex;
--- 313,320
* redundant, but it seems best to grab it anyway to ensure the index
* definition can't change under us.
*/
! bool
! check_index_is_clusterable(Relation OldHeap, Oid indexOid, int elevel)
{
RelationOldIndex;
***
*** 321,331
*/
if (OldIndex-rd_index == NULL ||
OldIndex-rd_index-indrelid != RelationGetRelid(OldHeap))
! ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg(\%s\ is not an index for table
\%s\,
RelationGetRelationName(OldIndex),
RelationGetRelationName(OldHeap;
/*
* Disallow clustering on incomplete indexes (those that might not
--- 326,340
*/
if (OldIndex-rd_index == NULL ||
OldIndex-rd_index-indrelid != RelationGetRelid(OldHeap))
! {
! ereport(elevel,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg(\%s\ is not an index for table
\%s\,
RelationGetRelationName(OldIndex),
Re: [PATCHES] Update psql and pg_dump for new COPY api
Actually, better not apply this - I think I've found some problems in it. Chris On Fri, 6 May 2005, Bruce Momjian wrote: Your patch has been added to the PostgreSQL unapplied patches list at: http://momjian.postgresql.org/cgi-bin/pgpatches It will be applied as soon as one of the PostgreSQL committers reviews and approves it. --- Christopher Kings-Lynne wrote: This patch updates psql and pg_dump to use the new copy api. Probably needs some review. I have tested it with dos and unix newlines, etc. Chris Content-Description: [ Attachment, skipping... ] ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED] -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] Dealing with CLUSTER failures
Seems like an idea to me... On another note, what about the problem I pointed out where it's not possible to drop the default on a serial column after you alter it to a varchar, for example... Chris On Sat, 7 May 2005, Bruce Momjian wrote: Christopher Kings-Lynne wrote: I don't think that's a bug. You may not intend ever to cluster on that index again, and if you try it will tell you about the problem. Except it breaks the 'cluster everything' case: test=# cluster; ERROR: cannot cluster when index access method does not handle null values HINT: You may be able to work around this by marking column a NOT NULL. I looked over this item, originally posted as: http://archives.postgresql.org/pgsql-hackers/2005-03/msg01055.php It seems that if you use an index method that doesn't support NULLs, you can use ALTER to set the column as NOT NULL, then CLUSTER, and then set it to allow NULLs, and when you CLUSTER all tables, the cluster errors out on that table. I thought about removing the cluster bit when you do the alter or something like that, but it seems too confusing and error-prone to be sure we get every case. I think the main problem is that while cluster is a performance-only feature, we error out if we can't cluster one table, basically treating it as though it needs transaction semantics. It doesn't. This patch throws an ERROR of you cluster a specific index that can't be clustered, but issues only a WARNING if you are clustering all tables. This allows it to report the failed cluster but keep going. I also modified the code to print the index name in case of failure, because without that the user doesn't know the failing index name in a database-wide cluster failure. Here is an example: test= cluster test_gist_idx on test; ERROR: cannot cluster on index test_gist_idx because access method does not handle null values HINT: You may be able to work around this by marking column a NOT NULL. test= cluster; WARNING: cannot cluster on index test_gist_idx because access method does not handle null values HINT: You may be able to work around this by marking column a NOT NULL. CLUSTER You can see the ERROR for a specific index, and WARNING for full database cluster. -- Bruce Momjian| http://candle.pha.pa.us pgman@candle.pha.pa.us | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] Dealing with CLUSTER failures
Bruce Momjian pgman@candle.pha.pa.us writes: I looked over this item, originally posted as: http://archives.postgresql.org/pgsql-hackers/2005-03/msg01055.php I still think this is substantial complication (more than likely introducing some bugs) to deal with a non problem. http://archives.postgresql.org/pgsql-hackers/2005-03/msg01058.php regards, tom lane ---(end of broadcast)--- TIP 2: you can get off all lists at once with the unregister command (send unregister YourEmailAddressHere to [EMAIL PROTECTED])
