pg_tablespace_databases

-----Original Message-----
From: pgsql-patches-owner@postgresql.org
[mailto:pgsql-patches-owner@postgresql.org] On Behalf Of Andreas Pflug
Sent: 28 June 2004 12:25
To: PostgreSQL Patches
Subject: [PATCHES] pg_tablespace_databases

From an idea of Bruce, the attached patch implements the function
pg_tablespace_databases(oid) RETURNS SETOF oid

which delivers as set of database oids having objects in the
selected tablespace, enabling an admin to examine only the
databases affecting the tablespace for objects instead of
scanning all of them.

Think you forgot the patch...

:-)

/D

Andreas Pflug wrote:

From an idea of Bruce, the attached patch implements the function
pg_tablespace_databases(oid) RETURNS SETOF oid

which delivers as set of database oids having objects in the selected
tablespace, enabling an admin to examine only the databases affecting
the tablespace for objects instead of scanning all of them.

It might be easier to review if I attach the file...

Regards,
Andreas

Andreas Pflug wrote:

From an idea of Bruce, the attached patch implements the function
pg_tablespace_databases(oid) RETURNS SETOF oid

which delivers as set of database oids having objects in the selected
tablespace, enabling an admin to examine only the databases affecting
the tablespace for objects instead of scanning all of them.

If there are no objections, I'll review and apply this tonight (west
coast USA time).

Andreas, please provide a corresponding documentation patch.

Thanks,

Joe

Joe Conway wrote:

If there are no objections, I'll review and apply this tonight (west
coast USA time).

Andreas, please provide a corresponding documentation patch.

Here we are.
Since I don't have a SGML build environment, syntax is not checked.

Regards,
Andreas

Andreas Pflug wrote:

From an idea of Bruce, the attached patch implements the function
pg_tablespace_databases(oid) RETURNS SETOF oid

which delivers as set of database oids having objects in the selected
tablespace, enabling an admin to examine only the databases affecting
the tablespace for objects instead of scanning all of them.

Attached is the patch I plan to apply. There are a couple of changes
from what was posted.

1) You must have meant tablespace instead of namespace here:
------------------------------------------------------------
+      <row>
+ 
<entry><literal><function>pg_tablespace_databases</function>(<parameter>namespace_oid</parameter>)</literal></entry>
+       <entry><type>setof oid</type></entry>
+       <entry>get set of database oids that have objects in the 
namespace</entry>
+      </row>

2) This allocation size was a bit ambigous and I think based on a once 
longer tablespace directory name:
------------------------------------------------------------
+		fctx->location = (char*)palloc(strlen(DataDir)+16+10+1);

I take it that is (path len + '/' + strlen("pg_tablespaces") + '/' + oid
string length + terminator). I did this instead:

+ #define PG_TABLESPACE_DIR	"pg_tblspc"
+ /* assumes unsigned, 10 digits */
+ #define OID_AS_STR	10

+ 		/*
+ 		 * size = path length + tablespace dirname length
+ 		 *        + 2 dir sep chars + oid + terminator
+ 		 */
+ 		fctx->location = (char*) palloc(strlen(DataDir)
+ 					+ strlen(PG_TABLESPACE_DIR)
+ 					+ 2 + OID_AS_STR + 1);

Usage looks like this:
regression=# select d.datname from pg_tablespace_databases(1663) as
t(oid) join pg_database d on t.oid = d.oid order by 1;
datname
------------
regression
template0
template1
(3 rows)

initdb forced.

Any objections?

Joe

Joe Conway wrote:

Attached is the patch I plan to apply. There are a couple of changes
from what was posted.

1) You must have meant tablespace instead of namespace here:
------------------------------------------------------------
+      <row>
+ 
<entry><literal><function>pg_tablespace_databases</function>(<parameter>namespace_oid</parameter>)</literal></entry> 

+ <entry><type>setof oid</type></entry>

Of course. I just call everything namespace :-)

2) This allocation size was a bit ambigous and I think based on a once 
longer tablespace directory name:
------------------------------------------------------------
+        fctx->location = (char*)palloc(strlen(DataDir)+16+10+1);

This size calculation originated (copy/paste) from
commands/tablespace.c, should be clarified there too (and "pg_tblspc" is
hardcoded in strings, could be extracted to a macro definition).

Regards,
Andreas

Andreas Pflug <pgadmin@pse-consulting.de> writes:

Joe Conway wrote:

2) This allocation size was a bit ambigous and I think based on a once
longer tablespace directory name:

This size calculation originated (copy/paste) from
commands/tablespace.c,

Yeah --- Bruce did not adjust the string length calculations when he
editorialized on the directory name. I'd been meaning to go back and
make them match.

should be clarified there too (and "pg_tblspc" is
hardcoded in strings, could be extracted to a macro definition).

[ shrug... ] The name is not going to change again. I have never cared
for the practice of writing strlen("foo") as if it were a compile-time
constant. But certainly it would be entirely pointless to define such a
macro and then use it in only one place.

regards, tom lane

Tom Lane wrote:

[ shrug... ] The name is not going to change again. I have never cared
for the practice of writing strlen("foo") as if it were a compile-time
constant. But certainly it would be entirely pointless to define such a
macro and then use it in only one place.

Fair enough. If there are no objections, I'll apply the attached patch
in a few hours.

Joe

Joe Conway wrote:

Tom Lane wrote:

[ shrug... ] The name is not going to change again. I have never cared
for the practice of writing strlen("foo") as if it were a compile-time
constant. But certainly it would be entirely pointless to define such a
macro and then use it in only one place.

Fair enough. If there are no objections, I'll apply the attached patch
in a few hours.

patch applied.

Joe

Tom Lane wrote:

Andreas Pflug <pgadmin@pse-consulting.de> writes:

Joe Conway wrote:

2) This allocation size was a bit ambigous and I think based on a once
longer tablespace directory name:

This size calculation originated (copy/paste) from
commands/tablespace.c,

Yeah --- Bruce did not adjust the string length calculations when he
editorialized on the directory name. I'd been meaning to go back and
make them match.

should be clarified there too (and "pg_tblspc" is
hardcoded in strings, could be extracted to a macro definition).

[ shrug... ] The name is not going to change again. I have never cared
for the practice of writing strlen("foo") as if it were a compile-time
constant. But certainly it would be entirely pointless to define such a
macro and then use it in only one place.

I think with gcc strlen("foo") is a compile-time constant. At least I
remember that as a gcc optimization. What do you prefer?
sizeof("foo")-1? Certainly +3 is poorly documented, no?

-- 
  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

On Tue, 6 Jul 2004, Bruce Momjian wrote:

Tom Lane wrote:

Andreas Pflug <pgadmin@pse-consulting.de> writes:

Joe Conway wrote:

2) This allocation size was a bit ambigous and I think based on a once
longer tablespace directory name:

This size calculation originated (copy/paste) from
commands/tablespace.c,

Yeah --- Bruce did not adjust the string length calculations when he
editorialized on the directory name. I'd been meaning to go back and
make them match.

should be clarified there too (and "pg_tblspc" is
hardcoded in strings, could be extracted to a macro definition).

[ shrug... ] The name is not going to change again. I have never cared
for the practice of writing strlen("foo") as if it were a compile-time
constant. But certainly it would be entirely pointless to define such a
macro and then use it in only one place.

I think with gcc strlen("foo") is a compile-time constant. At least I
remember that as a gcc optimization. What do you prefer?
sizeof("foo")-1? Certainly +3 is poorly documented, no?

You're right about the gcc optimisation:

int i = strlen("foo");
8048304: c7 45 fc 03 00 00 00 movl $0x3,0xfffffffc(%ebp)

It does look messy thought. Can't this be cleared by a comment?

Thanks,

Gavin

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

[ shrug... ] The name is not going to change again. I have never cared
for the practice of writing strlen("foo") as if it were a compile-time
constant.

I think with gcc strlen("foo") is a compile-time constant.

Portability is exactly the root of the problem. If you are in the habit
of doing this then you get led into unportable behaviors like
char localarray[strlen(foo) + 1];
which no compiler except gcc will take. (We just had to fix exactly
that mistake in someone's patch within the last week or two.)

What do you prefer?

I use "3" ;-). As long as the size calculation and the filling of the
string are immediately adjacent, the purpose of the code is clear
enough.

regards, tom lane

Tom Lane wrote:

Bruce Momjian <pgman@candle.pha.pa.us> writes:

Tom Lane wrote:

[ shrug... ] The name is not going to change again. I have never cared
for the practice of writing strlen("foo") as if it were a compile-time
constant.

I think with gcc strlen("foo") is a compile-time constant.

Portability is exactly the root of the problem. If you are in the habit
of doing this then you get led into unportable behaviors like
char localarray[strlen(foo) + 1];
which no compiler except gcc will take. (We just had to fix exactly
that mistake in someone's patch within the last week or two.)

One idea would be to create a CONST_STRLEN macro that uses sizeof()-1.

What do you prefer?

I use "3" ;-). As long as the size calculation and the filling of the
string are immediately adjacent, the purpose of the code is clear
enough.

If it is on the same line, yea, it is clear, but often the size refers
to something declared several lines away.

-- 
  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