Fix pgstattuple/pgstatindex to use regclass-type as the argument
Hi,
As I wrote before, I'd like to clean up pgstattuple functions to
allow the same expressions.
Re: [HACKERS] [RFC] pgstattuple/pgstatindex enhancement
/messages/by-id/511EE19B.5010004@uptime.jp
My goal is to allow specifying a relation/index with several
expressions, 'relname', 'schemaname.relname' and oid in all
pgstattuple functions. pgstatindex() does not accept oid so far.
I have found that the backward-compatibility can be kept
when the arguments (text and/or oid) are replaced with regclass
type. regclass type seems to be more appropriate here.
So, I cleaned up those three functions, pgstattuple(), pgstatindex(),
pg_relpages(), to accept a regclass-type argument, instead of using
text and/or oid type, as the test cases show.
select * from pgstatindex('test_pkey');
select * from pgstatindex('public.test_pkey');
select * from pgstatindex('myschema.test_pkey');
select * from pgstatindex('myschema.test_pkey'::regclass::oid);
With attached patch, all functions in the pgstattuple module can
accept the same expression to specify the target relation/index.
Any comments or suggestions?
Regards,
--
Satoshi Nagayasu <snaga@uptime.jp>
Uptime Technologies, LLC. http://www.uptime.jp
Attachments:
Satoshi Nagayasu <snaga@uptime.jp> writes:
My goal is to allow specifying a relation/index with several
expressions, 'relname', 'schemaname.relname' and oid in all
pgstattuple functions. pgstatindex() does not accept oid so far.
I have found that the backward-compatibility can be kept
when the arguments (text and/or oid) are replaced with regclass
type. regclass type seems to be more appropriate here.
I recall having looked at this with the same thought in mind, and
realizing that it's not really as simple as all that. Yes, it
will seem to be compatible in manual tests like
select * from pgstatindex('myschema.test_pkey');
but something like
select pgstattuple(relname) from pg_class where relkind = 'r';
will *not* work anymore, though it used to (modulo search path issues),
since there's no implicit cast from text to regclass.
Now of course, the above is very bad practice anyway --- it's much
safer, not to mention more efficient, to write
select pgstattuple(oid) from pg_class where relkind = 'r';
and that will still work if we replace the functions with a single
function taking regclass.
But ... historically, there hasn't been a pgstatindex(oid), and so
people may very well be using relname or perhaps oid::regclass::text
if they're using queries of this sort with pgstatindex.
Maybe this is acceptable collateral damage. I don't know. But we
definitely stand a chance of breaking applications if we change
pgstatindex like this. It might be better to invent a differently-named
function to replace pgstatindex.
Also, you can't just modify pgstattuple--1.1.sql like that. You have
to create pgstattuple--1.2.sql and provide an upgrade script. It'd be a
good idea also to make sure that the module doesn't dump core if used
with the old SQL function definitions.
regards, tom lane
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sun, Mar 3, 2013 at 5:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Maybe this is acceptable collateral damage. I don't know. But we
definitely stand a chance of breaking applications if we change
pgstatindex like this. It might be better to invent a differently-named
function to replace pgstatindex.
If this were a built-in function, we might have to make a painful
decision between breaking backward compatibility and leaving this
broken forever, but as it isn't, we don't. I think your suggestion of
adding a new function is exactly right. We can remove the old one in
a future release, and support both in the meantime. It strikes me
that if extension versioning is for anything, this is it.
We encountered, not long ago, a case where someone couldn't pg_upgrade
from 9.0 to 9.2. The reason is that they had defined a view which
happened to reference pg_stat_activity.procpid, which we renamed.
Oops. Granted, few users do that, and granted, we can't always
refrain from changing system catalog structure. But it seems to me
that it's good to avoid the pain where we can.
--
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
(2013/03/05 22:46), Robert Haas wrote:
On Sun, Mar 3, 2013 at 5:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Maybe this is acceptable collateral damage. I don't know. But we
definitely stand a chance of breaking applications if we change
pgstatindex like this. It might be better to invent a differently-named
function to replace pgstatindex.If this were a built-in function, we might have to make a painful
decision between breaking backward compatibility and leaving this
broken forever, but as it isn't, we don't. I think your suggestion of
adding a new function is exactly right. We can remove the old one in
a future release, and support both in the meantime. It strikes me
that if extension versioning is for anything, this is it.
It is obviously easy to keep two types of function interfaces,
one with regclass-type and another with text-type, in the next
release for backward-compatibility like below:
pgstattuple(regclass) -- safer interface.
pgstattuple(text) -- will be depreciated in the future release.
Having both interfaces for a while would allow users to have enough
time to rewrite their applications.
Then, we will be able to obsolete (or just drop) old interfaces
in the future release, maybe 9.4 or 9.5. I think this approach
would minimize an impact of such interface change.
So, I think we can clean up function arguments in the pgstattuple
module, and also we can have two interfaces, both regclass and text,
for the next release.
Any comments?
Regards,
--
Satoshi Nagayasu <snaga@uptime.jp>
Uptime Technologies, LLC. http://www.uptime.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu <snaga@uptime.jp> wrote:
(2013/03/05 22:46), Robert Haas wrote:
On Sun, Mar 3, 2013 at 5:32 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Maybe this is acceptable collateral damage. I don't know. But we
definitely stand a chance of breaking applications if we change
pgstatindex like this. It might be better to invent a differently-named
function to replace pgstatindex.If this were a built-in function, we might have to make a painful
decision between breaking backward compatibility and leaving this
broken forever, but as it isn't, we don't. I think your suggestion of
adding a new function is exactly right. We can remove the old one in
a future release, and support both in the meantime. It strikes me
that if extension versioning is for anything, this is it.It is obviously easy to keep two types of function interfaces,
one with regclass-type and another with text-type, in the next
release for backward-compatibility like below:pgstattuple(regclass) -- safer interface.
pgstattuple(text) -- will be depreciated in the future release.
So you're thinking to remove pgstattuple(oid) soon?
Having both interfaces for a while would allow users to have enough
time to rewrite their applications.Then, we will be able to obsolete (or just drop) old interfaces
in the future release, maybe 9.4 or 9.5. I think this approach
would minimize an impact of such interface change.So, I think we can clean up function arguments in the pgstattuple
module, and also we can have two interfaces, both regclass and text,
for the next release.Any comments?
In the document, you should mark old functions as deprecated.
I changed the status of this patch to "Waiting on Author".
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
(2013/06/17 4:02), Fujii Masao wrote:
On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu <snaga@uptime.jp> wrote:
It is obviously easy to keep two types of function interfaces,
one with regclass-type and another with text-type, in the next
release for backward-compatibility like below:pgstattuple(regclass) -- safer interface.
pgstattuple(text) -- will be depreciated in the future release.So you're thinking to remove pgstattuple(oid) soon?
AFAIK, a regclass type argument would accept an OID value,
which means regclass type has upper-compatibility against
oid type.
So, even if the declaration is changed, compatibility could
be kept actually. This test case (in sql/pgstattuple.sql)
confirms that.
----------------------------------------------------------------
select * from pgstatindex('myschema.test_pkey'::regclass::oid);
version | tree_level | index_size | root_block_no | internal_pages |
leaf_pages | empty_pages | deleted_pages | avg_leaf_density |
leaf_fragmentation
---------+------------+------------+---------------+----------------+------------+-------------+---------------+------------------+--------------------
2 | 0 | 8192 | 1 | 0 |
1 | 0 | 0 | 0.79 |
0
(1 row)
----------------------------------------------------------------
Having both interfaces for a while would allow users to have enough
time to rewrite their applications.Then, we will be able to obsolete (or just drop) old interfaces
in the future release, maybe 9.4 or 9.5. I think this approach
would minimize an impact of such interface change.So, I think we can clean up function arguments in the pgstattuple
module, and also we can have two interfaces, both regclass and text,
for the next release.Any comments?
In the document, you should mark old functions as deprecated.
I'm still considering changing the function name as Tom pointed
out. How about "pgstatbtindex"?
In fact, pgstatindex does support only BTree index.
So, "pgstatbtindex" seems to be more appropriate for this function.
We can keep having both (old) pgstatindex and (new) pgstatbtindex
during next 2-3 major releases, and the old one will be deprecated
after that.
Any comments?
Regards,
--
Satoshi Nagayasu <snaga@uptime.jp>
Uptime Technologies, LLC. http://www.uptime.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 20, 2013 at 11:43 AM, Satoshi Nagayasu <snaga@uptime.jp> wrote:
(2013/06/17 4:02), Fujii Masao wrote:
On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu <snaga@uptime.jp> wrote:
It is obviously easy to keep two types of function interfaces,
one with regclass-type and another with text-type, in the next
release for backward-compatibility like below:pgstattuple(regclass) -- safer interface.
pgstattuple(text) -- will be depreciated in the future release.So you're thinking to remove pgstattuple(oid) soon?
AFAIK, a regclass type argument would accept an OID value,
which means regclass type has upper-compatibility against
oid type.So, even if the declaration is changed, compatibility could
be kept actually. This test case (in sql/pgstattuple.sql)
confirms that.----------------------------------------------------------------
select * from pgstatindex('myschema.test_pkey'::regclass::oid);
version | tree_level | index_size | root_block_no | internal_pages |
leaf_pages | empty_pages | deleted_pages | avg_leaf_density |
leaf_fragmentation
---------+------------+------------+---------------+----------------+------------+-------------+---------------+------------------+--------------------
2 | 0 | 8192 | 1 | 0 |
1 | 0 | 0 | 0.79 | 0
(1 row)
----------------------------------------------------------------Having both interfaces for a while would allow users to have enough
time to rewrite their applications.Then, we will be able to obsolete (or just drop) old interfaces
in the future release, maybe 9.4 or 9.5. I think this approach
would minimize an impact of such interface change.So, I think we can clean up function arguments in the pgstattuple
module, and also we can have two interfaces, both regclass and text,
for the next release.Any comments?
In the document, you should mark old functions as deprecated.
I'm still considering changing the function name as Tom pointed
out. How about "pgstatbtindex"?
Or just adding pgstatindex(regclass)?
In fact, pgstatindex does support only BTree index.
So, "pgstatbtindex" seems to be more appropriate for this function.
Can most ordinary users realize "bt" means "btree"?
We can keep having both (old) pgstatindex and (new) pgstatbtindex
during next 2-3 major releases, and the old one will be deprecated
after that.
Since pg_relpages(oid) doesn't exist, pg_relpages() is in the same
situation as pgstatindex(), i.e., we cannot just replace pg_relpages(text)
with pg_relpages(regclass) for the backward-compatibility. How do you
think we should solve the pg_relpages() problem? Rename? Just
add pg_relpages(regclass)?
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Thu, Jun 20, 2013 at 2:32 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Thu, Jun 20, 2013 at 11:43 AM, Satoshi Nagayasu <snaga@uptime.jp> wrote:
(2013/06/17 4:02), Fujii Masao wrote:
On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu <snaga@uptime.jp> wrote:
It is obviously easy to keep two types of function interfaces,
one with regclass-type and another with text-type, in the next
release for backward-compatibility like below:pgstattuple(regclass) -- safer interface.
pgstattuple(text) -- will be depreciated in the future release.So you're thinking to remove pgstattuple(oid) soon?
AFAIK, a regclass type argument would accept an OID value,
which means regclass type has upper-compatibility against
oid type.So, even if the declaration is changed, compatibility could
be kept actually. This test case (in sql/pgstattuple.sql)
confirms that.----------------------------------------------------------------
select * from pgstatindex('myschema.test_pkey'::regclass::oid);
version | tree_level | index_size | root_block_no | internal_pages |
leaf_pages | empty_pages | deleted_pages | avg_leaf_density |
leaf_fragmentation
---------+------------+------------+---------------+----------------+------------+-------------+---------------+------------------+--------------------
2 | 0 | 8192 | 1 | 0 |
1 | 0 | 0 | 0.79 | 0
(1 row)
----------------------------------------------------------------Having both interfaces for a while would allow users to have enough
time to rewrite their applications.Then, we will be able to obsolete (or just drop) old interfaces
in the future release, maybe 9.4 or 9.5. I think this approach
would minimize an impact of such interface change.So, I think we can clean up function arguments in the pgstattuple
module, and also we can have two interfaces, both regclass and text,
for the next release.Any comments?
In the document, you should mark old functions as deprecated.
I'm still considering changing the function name as Tom pointed
out. How about "pgstatbtindex"?Or just adding pgstatindex(regclass)?
In fact, pgstatindex does support only BTree index.
So, "pgstatbtindex" seems to be more appropriate for this function.Can most ordinary users realize "bt" means "btree"?
We can keep having both (old) pgstatindex and (new) pgstatbtindex
during next 2-3 major releases, and the old one will be deprecated
after that.Since pg_relpages(oid) doesn't exist, pg_relpages() is in the same
situation as pgstatindex(), i.e., we cannot just replace pg_relpages(text)
with pg_relpages(regclass) for the backward-compatibility. How do you
think we should solve the pg_relpages() problem? Rename? Just
add pg_relpages(regclass)?
Adding a function with a new name seems likely to be smoother, since
that way you don't have to worry about problems with function calls
being thought ambiguous.
--
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
On Wed, Jun 26, 2013 at 12:39 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jun 20, 2013 at 2:32 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Thu, Jun 20, 2013 at 11:43 AM, Satoshi Nagayasu <snaga@uptime.jp> wrote:
(2013/06/17 4:02), Fujii Masao wrote:
On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu <snaga@uptime.jp> wrote:
It is obviously easy to keep two types of function interfaces,
one with regclass-type and another with text-type, in the next
release for backward-compatibility like below:pgstattuple(regclass) -- safer interface.
pgstattuple(text) -- will be depreciated in the future release.So you're thinking to remove pgstattuple(oid) soon?
AFAIK, a regclass type argument would accept an OID value,
which means regclass type has upper-compatibility against
oid type.So, even if the declaration is changed, compatibility could
be kept actually. This test case (in sql/pgstattuple.sql)
confirms that.----------------------------------------------------------------
select * from pgstatindex('myschema.test_pkey'::regclass::oid);
version | tree_level | index_size | root_block_no | internal_pages |
leaf_pages | empty_pages | deleted_pages | avg_leaf_density |
leaf_fragmentation
---------+------------+------------+---------------+----------------+------------+-------------+---------------+------------------+--------------------
2 | 0 | 8192 | 1 | 0 |
1 | 0 | 0 | 0.79 | 0
(1 row)
----------------------------------------------------------------Having both interfaces for a while would allow users to have enough
time to rewrite their applications.Then, we will be able to obsolete (or just drop) old interfaces
in the future release, maybe 9.4 or 9.5. I think this approach
would minimize an impact of such interface change.So, I think we can clean up function arguments in the pgstattuple
module, and also we can have two interfaces, both regclass and text,
for the next release.Any comments?
In the document, you should mark old functions as deprecated.
I'm still considering changing the function name as Tom pointed
out. How about "pgstatbtindex"?Or just adding pgstatindex(regclass)?
In fact, pgstatindex does support only BTree index.
So, "pgstatbtindex" seems to be more appropriate for this function.Can most ordinary users realize "bt" means "btree"?
We can keep having both (old) pgstatindex and (new) pgstatbtindex
during next 2-3 major releases, and the old one will be deprecated
after that.Since pg_relpages(oid) doesn't exist, pg_relpages() is in the same
situation as pgstatindex(), i.e., we cannot just replace pg_relpages(text)
with pg_relpages(regclass) for the backward-compatibility. How do you
think we should solve the pg_relpages() problem? Rename? Just
add pg_relpages(regclass)?Adding a function with a new name seems likely to be smoother, since
that way you don't have to worry about problems with function calls
being thought ambiguous.
Could you let me know the example that this problem happens?
For the test, I just implemented the regclass-version of pg_relpages()
(patch attached) and tested some cases. But I could not get that problem.
SELECT pg_relpages('hoge'); -- OK
SELECT pg_relpages(oid) FROM pg_class WHERE relname = 'hoge'; -- OK
SELECT pg_relpages(relname) FROM pg_class WHERE relname = 'hoge'; -- OK
Regards,
--
Fujii Masao
Attachments:
pg_relpages_test.patchapplication/octet-stream; name=pg_relpages_test.patchDownload
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 97f897e..4499f29 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -41,10 +41,12 @@
extern Datum pgstatindex(PG_FUNCTION_ARGS);
extern Datum pg_relpages(PG_FUNCTION_ARGS);
+extern Datum pg_relpages_regclass(PG_FUNCTION_ARGS);
extern Datum pgstatginindex(PG_FUNCTION_ARGS);
PG_FUNCTION_INFO_V1(pgstatindex);
PG_FUNCTION_INFO_V1(pg_relpages);
+PG_FUNCTION_INFO_V1(pg_relpages_regclass);
PG_FUNCTION_INFO_V1(pgstatginindex);
#define IS_INDEX(r) ((r)->rd_rel->relkind == RELKIND_INDEX)
@@ -311,6 +313,29 @@ pg_relpages(PG_FUNCTION_ARGS)
PG_RETURN_INT64(relpages);
}
+Datum
+pg_relpages_regclass(PG_FUNCTION_ARGS)
+{
+ Oid relid = PG_GETARG_OID(0);
+ int64 relpages;
+ Relation rel;
+
+ if (!superuser())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ (errmsg("must be superuser to use pgstattuple functions"))));
+
+ rel = relation_open(relid, AccessShareLock);
+
+ /* note: this will work OK on non-local temp tables */
+
+ relpages = RelationGetNumberOfBlocks(rel);
+
+ relation_close(rel, AccessShareLock);
+
+ PG_RETURN_INT64(relpages);
+}
+
/* ------------------------------------------------------
* pgstatginindex()
*
diff --git a/contrib/pgstattuple/pgstattuple--1.1.sql b/contrib/pgstattuple/pgstattuple--1.1.sql
index b21fbf8..21f467b 100644
--- a/contrib/pgstattuple/pgstattuple--1.1.sql
+++ b/contrib/pgstattuple/pgstattuple--1.1.sql
@@ -48,6 +48,11 @@ RETURNS BIGINT
AS 'MODULE_PATHNAME', 'pg_relpages'
LANGUAGE C STRICT;
+CREATE FUNCTION pg_relpages(IN relname regclass)
+RETURNS BIGINT
+AS 'MODULE_PATHNAME', 'pg_relpages_regclass'
+LANGUAGE C STRICT;
+
/* New stuff in 1.1 begins here */
CREATE FUNCTION pgstatginindex(IN relname regclass,
Hello Satoshi,
I assigned myself for the reviewer of this patch. Issue status is waiting on
author.
Now looking at the discussion under the thread it seems like we are waiting
for the suggestion for the new function name, right ?
I am wondering why actually we need new name ? Can't we just overload the
same function and provide two version of the functions ?
In the last thread Fujii just did the same for pg_relpages and it seems
like an
good to go approach, isn't it ? Am I missing anything here ?
Regards,
On Thu, Jul 4, 2013 at 12:28 AM, Fujii Masao <masao.fujii@gmail.com> wrote:
On Wed, Jun 26, 2013 at 12:39 AM, Robert Haas <robertmhaas@gmail.com>
wrote:On Thu, Jun 20, 2013 at 2:32 PM, Fujii Masao <masao.fujii@gmail.com>
wrote:
On Thu, Jun 20, 2013 at 11:43 AM, Satoshi Nagayasu <snaga@uptime.jp>
wrote:
(2013/06/17 4:02), Fujii Masao wrote:
On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu <snaga@uptime.jp>
wrote:
It is obviously easy to keep two types of function interfaces,
one with regclass-type and another with text-type, in the next
release for backward-compatibility like below:pgstattuple(regclass) -- safer interface.
pgstattuple(text) -- will be depreciated in the future release.So you're thinking to remove pgstattuple(oid) soon?
AFAIK, a regclass type argument would accept an OID value,
which means regclass type has upper-compatibility against
oid type.So, even if the declaration is changed, compatibility could
be kept actually. This test case (in sql/pgstattuple.sql)
confirms that.----------------------------------------------------------------
select * from pgstatindex('myschema.test_pkey'::regclass::oid);
version | tree_level | index_size | root_block_no | internal_pages |
leaf_pages | empty_pages | deleted_pages | avg_leaf_density |
leaf_fragmentation---------+------------+------------+---------------+----------------+------------+-------------+---------------+------------------+--------------------
2 | 0 | 8192 | 1 | 0 |
1 | 0 | 0 | 0.79 | 0
(1 row)
----------------------------------------------------------------Having both interfaces for a while would allow users to have enough
time to rewrite their applications.Then, we will be able to obsolete (or just drop) old interfaces
in the future release, maybe 9.4 or 9.5. I think this approach
would minimize an impact of such interface change.So, I think we can clean up function arguments in the pgstattuple
module, and also we can have two interfaces, both regclass and text,
for the next release.Any comments?
In the document, you should mark old functions as deprecated.
I'm still considering changing the function name as Tom pointed
out. How about "pgstatbtindex"?Or just adding pgstatindex(regclass)?
In fact, pgstatindex does support only BTree index.
So, "pgstatbtindex" seems to be more appropriate for this function.Can most ordinary users realize "bt" means "btree"?
We can keep having both (old) pgstatindex and (new) pgstatbtindex
during next 2-3 major releases, and the old one will be deprecated
after that.Since pg_relpages(oid) doesn't exist, pg_relpages() is in the same
situation as pgstatindex(), i.e., we cannot just replacepg_relpages(text)
with pg_relpages(regclass) for the backward-compatibility. How do you
think we should solve the pg_relpages() problem? Rename? Just
add pg_relpages(regclass)?Adding a function with a new name seems likely to be smoother, since
that way you don't have to worry about problems with function calls
being thought ambiguous.Could you let me know the example that this problem happens?
For the test, I just implemented the regclass-version of pg_relpages()
(patch attached) and tested some cases. But I could not get that problem.SELECT pg_relpages('hoge'); -- OK
SELECT pg_relpages(oid) FROM pg_class WHERE relname = 'hoge'; -- OK
SELECT pg_relpages(relname) FROM pg_class WHERE relname = 'hoge';
-- OKRegards,
--
Fujii Masao--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
--
Rushabh Lathia
(2013/07/04 3:58), Fujii Masao wrote:
On Wed, Jun 26, 2013 at 12:39 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jun 20, 2013 at 2:32 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
Since pg_relpages(oid) doesn't exist, pg_relpages() is in the same
situation as pgstatindex(), i.e., we cannot just replace pg_relpages(text)
with pg_relpages(regclass) for the backward-compatibility. How do you
think we should solve the pg_relpages() problem? Rename? Just
add pg_relpages(regclass)?Adding a function with a new name seems likely to be smoother, since
that way you don't have to worry about problems with function calls
being thought ambiguous.Could you let me know the example that this problem happens?
For the test, I just implemented the regclass-version of pg_relpages()
(patch attached) and tested some cases. But I could not get that problem.SELECT pg_relpages('hoge'); -- OK
SELECT pg_relpages(oid) FROM pg_class WHERE relname = 'hoge'; -- OK
SELECT pg_relpages(relname) FROM pg_class WHERE relname = 'hoge'; -- OK
In the attached patch, I cleaned up three functions to have
two types of arguments for each, text and regclass.
pgstattuple(text)
pgstattuple(regclass)
pgstatindex(text)
pgstatindex(regclass)
pg_relpages(text)
pg_relpages(regclass)
I still think a regclass argument is more appropriate for passing
relation/index name to a function than text-type, but having both
arguments in each function seems to be a good choice at this moment,
in terms of backward-compatibility.
Docs needs to be updated if this change going to be applied.
Any comments?
--
Satoshi Nagayasu <snaga@uptime.jp>
Uptime Technologies, LLC. http://www.uptime.jp
Attachments:
pgstattuple_regclass_v2.difftext/plain; charset=Shift_JIS; name=pgstattuple_regclass_v2.diffDownload
diff --git a/contrib/pgstattuple/Makefile b/contrib/pgstattuple/Makefile
index fc893d8..957742a 100644
--- a/contrib/pgstattuple/Makefile
+++ b/contrib/pgstattuple/Makefile
@@ -4,7 +4,7 @@ MODULE_big = pgstattuple
OBJS = pgstattuple.o pgstatindex.o
EXTENSION = pgstattuple
-DATA = pgstattuple--1.1.sql pgstattuple--1.0--1.1.sql pgstattuple--unpackaged--1.0.sql
+DATA = pgstattuple--1.2.sql pgstattuple--1.0--1.1.sql pgstattuple--unpackaged--1.0.sql
REGRESS = pgstattuple
diff --git a/contrib/pgstattuple/expected/pgstattuple.out b/contrib/pgstattuple/expected/pgstattuple.out
index ab28f50..eaba306 100644
--- a/contrib/pgstattuple/expected/pgstattuple.out
+++ b/contrib/pgstattuple/expected/pgstattuple.out
@@ -11,12 +11,24 @@ select * from pgstattuple('test'::text);
0 | 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0
(1 row)
+select * from pgstattuple('test'::name);
+ table_len | tuple_count | tuple_len | tuple_percent | dead_tuple_count | dead_tuple_len | dead_tuple_percent | free_space | free_percent
+-----------+-------------+-----------+---------------+------------------+----------------+--------------------+------------+--------------
+ 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0
+(1 row)
+
select * from pgstattuple('test'::regclass);
table_len | tuple_count | tuple_len | tuple_percent | dead_tuple_count | dead_tuple_len | dead_tuple_percent | free_space | free_percent
-----------+-------------+-----------+---------------+------------------+----------------+--------------------+------------+--------------
0 | 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0
(1 row)
+select * from pgstattuple('test'::regclass::oid);
+ table_len | tuple_count | tuple_len | tuple_percent | dead_tuple_count | dead_tuple_len | dead_tuple_percent | free_space | free_percent
+-----------+-------------+-----------+---------------+------------------+----------------+--------------------+------------+--------------
+ 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0
+(1 row)
+
select * from pgstatindex('test_pkey');
version | tree_level | index_size | root_block_no | internal_pages | leaf_pages | empty_pages | deleted_pages | avg_leaf_density | leaf_fragmentation
---------+------------+------------+---------------+----------------+------------+-------------+---------------+------------------+--------------------
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 97f897e..9ec74e7 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -40,11 +40,15 @@
extern Datum pgstatindex(PG_FUNCTION_ARGS);
+extern Datum pgstatindexbyid(PG_FUNCTION_ARGS);
extern Datum pg_relpages(PG_FUNCTION_ARGS);
+extern Datum pg_relpagesbyid(PG_FUNCTION_ARGS);
extern Datum pgstatginindex(PG_FUNCTION_ARGS);
PG_FUNCTION_INFO_V1(pgstatindex);
+PG_FUNCTION_INFO_V1(pgstatindexbyid);
PG_FUNCTION_INFO_V1(pg_relpages);
+PG_FUNCTION_INFO_V1(pg_relpagesbyid);
PG_FUNCTION_INFO_V1(pgstatginindex);
#define IS_INDEX(r) ((r)->rd_rel->relkind == RELKIND_INDEX)
@@ -97,6 +101,8 @@ typedef struct GinIndexStat
int64 pending_tuples;
} GinIndexStat;
+static Datum pgstatindex_impl(Relation, FunctionCallInfo);
+
/* ------------------------------------------------------
* pgstatindex()
*
@@ -109,11 +115,6 @@ pgstatindex(PG_FUNCTION_ARGS)
text *relname = PG_GETARG_TEXT_P(0);
Relation rel;
RangeVar *relrv;
- Datum result;
- BlockNumber nblocks;
- BlockNumber blkno;
- BTIndexStat indexStat;
- BufferAccessStrategy bstrategy = GetAccessStrategy(BAS_BULKREAD);
if (!superuser())
ereport(ERROR,
@@ -123,6 +124,34 @@ pgstatindex(PG_FUNCTION_ARGS)
relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
rel = relation_openrv(relrv, AccessShareLock);
+ PG_RETURN_DATUM( pgstatindex_impl(rel, fcinfo) );
+}
+
+Datum
+pgstatindexbyid(PG_FUNCTION_ARGS)
+{
+ Oid relid = PG_GETARG_OID(0);
+ Relation rel;
+
+ if (!superuser())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ (errmsg("must be superuser to use pgstattuple functions"))));
+
+ rel = relation_open(relid, AccessShareLock);
+
+ PG_RETURN_DATUM( pgstatindex_impl(rel, fcinfo) );
+}
+
+static Datum
+pgstatindex_impl(Relation rel, FunctionCallInfo fcinfo)
+{
+ Datum result;
+ BlockNumber nblocks;
+ BlockNumber blkno;
+ BTIndexStat indexStat;
+ BufferAccessStrategy bstrategy = GetAccessStrategy(BAS_BULKREAD);
+
if (!IS_INDEX(rel) || !IS_BTREE(rel))
elog(ERROR, "relation \"%s\" is not a btree index",
RelationGetRelationName(rel));
@@ -274,7 +303,7 @@ pgstatindex(PG_FUNCTION_ARGS)
result = HeapTupleGetDatum(tuple);
}
- PG_RETURN_DATUM(result);
+ return result;
}
/* --------------------------------------------------------
@@ -311,6 +340,29 @@ pg_relpages(PG_FUNCTION_ARGS)
PG_RETURN_INT64(relpages);
}
+Datum
+pg_relpagesbyid(PG_FUNCTION_ARGS)
+{
+ Oid relid = PG_GETARG_OID(0);
+ int64 relpages;
+ Relation rel;
+
+ if (!superuser())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ (errmsg("must be superuser to use pgstattuple functions"))));
+
+ rel = relation_open(relid, AccessShareLock);
+
+ /* note: this will work OK on non-local temp tables */
+
+ relpages = RelationGetNumberOfBlocks(rel);
+
+ relation_close(rel, AccessShareLock);
+
+ PG_RETURN_INT64(relpages);
+}
+
/* ------------------------------------------------------
* pgstatginindex()
*
diff --git a/contrib/pgstattuple/pgstattuple--1.2.sql b/contrib/pgstattuple/pgstattuple--1.2.sql
new file mode 100644
index 0000000..a442e6a
--- /dev/null
+++ b/contrib/pgstattuple/pgstattuple--1.2.sql
@@ -0,0 +1,77 @@
+/* contrib/pgstattuple/pgstattuple--1.1.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION pgstattuple" to load this file. \quit
+
+CREATE FUNCTION pgstattuple(IN relname text,
+ OUT table_len BIGINT, -- physical table length in bytes
+ OUT tuple_count BIGINT, -- number of live tuples
+ OUT tuple_len BIGINT, -- total tuples length in bytes
+ OUT tuple_percent FLOAT8, -- live tuples in %
+ OUT dead_tuple_count BIGINT, -- number of dead tuples
+ OUT dead_tuple_len BIGINT, -- total dead tuples length in bytes
+ OUT dead_tuple_percent FLOAT8, -- dead tuples in %
+ OUT free_space BIGINT, -- free space in bytes
+ OUT free_percent FLOAT8) -- free space in %
+AS 'MODULE_PATHNAME', 'pgstattuple'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION pgstattuple(IN reloid regclass,
+ OUT table_len BIGINT, -- physical table length in bytes
+ OUT tuple_count BIGINT, -- number of live tuples
+ OUT tuple_len BIGINT, -- total tuples length in bytes
+ OUT tuple_percent FLOAT8, -- live tuples in %
+ OUT dead_tuple_count BIGINT, -- number of dead tuples
+ OUT dead_tuple_len BIGINT, -- total dead tuples length in bytes
+ OUT dead_tuple_percent FLOAT8, -- dead tuples in %
+ OUT free_space BIGINT, -- free space in bytes
+ OUT free_percent FLOAT8) -- free space in %
+AS 'MODULE_PATHNAME', 'pgstattuplebyid'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION pgstatindex(IN relname text,
+ OUT version INT,
+ OUT tree_level INT,
+ OUT index_size BIGINT,
+ OUT root_block_no BIGINT,
+ OUT internal_pages BIGINT,
+ OUT leaf_pages BIGINT,
+ OUT empty_pages BIGINT,
+ OUT deleted_pages BIGINT,
+ OUT avg_leaf_density FLOAT8,
+ OUT leaf_fragmentation FLOAT8)
+AS 'MODULE_PATHNAME', 'pgstatindex'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION pgstatindex(IN relname regclass,
+ OUT version INT,
+ OUT tree_level INT,
+ OUT index_size BIGINT,
+ OUT root_block_no BIGINT,
+ OUT internal_pages BIGINT,
+ OUT leaf_pages BIGINT,
+ OUT empty_pages BIGINT,
+ OUT deleted_pages BIGINT,
+ OUT avg_leaf_density FLOAT8,
+ OUT leaf_fragmentation FLOAT8)
+AS 'MODULE_PATHNAME', 'pgstatindexbyid'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION pg_relpages(IN relname text)
+RETURNS BIGINT
+AS 'MODULE_PATHNAME', 'pg_relpages'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION pg_relpages(IN relname regclass)
+RETURNS BIGINT
+AS 'MODULE_PATHNAME', 'pg_relpagesbyid'
+LANGUAGE C STRICT;
+
+/* New stuff in 1.1 begins here */
+
+CREATE FUNCTION pgstatginindex(IN relname regclass,
+ OUT version INT4,
+ OUT pending_pages INT4,
+ OUT pending_tuples BIGINT)
+AS 'MODULE_PATHNAME', 'pgstatginindex'
+LANGUAGE C STRICT;
diff --git a/contrib/pgstattuple/pgstattuple.control b/contrib/pgstattuple/pgstattuple.control
index fcfd36f..a7cf47f 100644
--- a/contrib/pgstattuple/pgstattuple.control
+++ b/contrib/pgstattuple/pgstattuple.control
@@ -1,5 +1,5 @@
# pgstattuple extension
comment = 'show tuple-level statistics'
-default_version = '1.1'
+default_version = '1.2'
module_pathname = '$libdir/pgstattuple'
relocatable = true
diff --git a/contrib/pgstattuple/sql/pgstattuple.sql b/contrib/pgstattuple/sql/pgstattuple.sql
index 8cb350d..76369c5 100644
--- a/contrib/pgstattuple/sql/pgstattuple.sql
+++ b/contrib/pgstattuple/sql/pgstattuple.sql
@@ -9,12 +9,23 @@ CREATE EXTENSION pgstattuple;
create table test (a int primary key, b int[]);
select * from pgstattuple('test'::text);
+select * from pgstattuple('test'::name);
select * from pgstattuple('test'::regclass);
+select * from pgstattuple('test'::regclass::oid);
+select pgstattuple(relname) from pg_class where relname = 'test' and relkind = 'r';
-select * from pgstatindex('test_pkey');
+select * from pgstatindex('test_pkey'::text);
+select * from pgstatindex('test_pkey'::name);
+select * from pgstatindex('test_pkey'::regclass);
+select * from pgstatindex('test_pkey'::regclass::oid);
+select pgstatindex(relname) from pg_class where relname = 'test_pkey' and relkind = 'i';
select pg_relpages('test');
select pg_relpages('test_pkey');
+select pg_relpages('test_pkey'::name);
+select pg_relpages('test_pkey'::regclass);
+select pg_relpages('test_pkey'::regclass::oid);
+select pg_relpages(relname) from pg_class where relname = 'test_pkey' and relkind = 'i';
create index test_ginidx on test using gin (b);
Hi Rushabh,
(2013/07/16 14:58), Rushabh Lathia wrote:
Hello Satoshi,
I assigned myself for the reviewer of this patch. Issue status is waiting on
author.
Thank you for picking it up.
Now looking at the discussion under the thread it seems like we are waiting
for the suggestion for the new function name, right ?
Yes.
I am wondering why actually we need new name ? Can't we just overload the
same function and provide two version of the functions ?
I think the major reason is to avoid some confusion with old and new
function arguments.
My thought here is that having both arguments (text and regclass)
for each function is a good choice to clean up interfaces with keeping
the backward-compatibility.
In the last thread Fujii just did the same for pg_relpages and it seems
like an
good to go approach, isn't it ? Am I missing anything here ?
I just posted a revised patch to handle the issue in three functions
of the pgstattuple module. Please take a look.
Regards,
--
Satoshi Nagayasu <snaga@uptime.jp>
Uptime Technologies, LLC. http://www.uptime.jp
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Hi Satoshi,
I spent some time on the revised version on the
patch(pgstattuple_regclass_v2.diff)
and here are my comments.
.) Patch get applies cleanly on PG master branch
.) Successful build and database creation
.) Basic test coverage included in the patch
.) make check running cleanly
Basically goal of the patch is to allow specifying a relation/index with
several expressions, 'relname', 'schemaname.relname' and oid in all
pgstattuple
functions. To achieve the same patch introduced another version of
pgstattuple
functions which takes regclass as input args. To make it backward compatible
we kept the pgstatetuple functions with TEXT input arg.
In the mail thread we decided that pgstattuple(text) will be depreciated in
the future release and we need to document that. Which is missing in the
patch.
Apart from that few comments in the C code to explain "why multiple version
of the pgstattuple function ?" would be really helpful for future
understanding
purpose.
Thanks,
On Tue, Jul 16, 2013 at 11:42 AM, Satoshi Nagayasu <snaga@uptime.jp> wrote:
Hi Rushabh,
(2013/07/16 14:58), Rushabh Lathia wrote:
Hello Satoshi,
I assigned myself for the reviewer of this patch. Issue status is waiting
on
author.Thank you for picking it up.
Now looking at the discussion under the thread it seems like we are
waiting
for the suggestion for the new function name, right ?Yes.
I am wondering why actually we need new name ? Can't we just overload the
same function and provide two version of the functions ?
I think the major reason is to avoid some confusion with old and new
function arguments.My thought here is that having both arguments (text and regclass)
for each function is a good choice to clean up interfaces with keeping
the backward-compatibility.In the last thread Fujii just did the same for pg_relpages and it seems
like an
good to go approach, isn't it ? Am I missing anything here ?I just posted a revised patch to handle the issue in three functions
of the pgstattuple module. Please take a look.Regards,
--
Satoshi Nagayasu <snaga@uptime.jp>
Uptime Technologies, LLC. http://www.uptime.jp
--
Rushabh Lathia
On Tue, Jul 16, 2013 at 3:00 PM, Satoshi Nagayasu <snaga@uptime.jp> wrote:
(2013/07/04 3:58), Fujii Masao wrote:
On Wed, Jun 26, 2013 at 12:39 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Jun 20, 2013 at 2:32 PM, Fujii Masao <masao.fujii@gmail.com> wrote:
Since pg_relpages(oid) doesn't exist, pg_relpages() is in the same
situation as pgstatindex(), i.e., we cannot just replace pg_relpages(text)
with pg_relpages(regclass) for the backward-compatibility. How do you
think we should solve the pg_relpages() problem? Rename? Just
add pg_relpages(regclass)?Adding a function with a new name seems likely to be smoother, since
that way you don't have to worry about problems with function calls
being thought ambiguous.Could you let me know the example that this problem happens?
For the test, I just implemented the regclass-version of pg_relpages()
(patch attached) and tested some cases. But I could not get that problem.SELECT pg_relpages('hoge'); -- OK
SELECT pg_relpages(oid) FROM pg_class WHERE relname = 'hoge'; -- OK
SELECT pg_relpages(relname) FROM pg_class WHERE relname = 'hoge'; -- OKIn the attached patch, I cleaned up three functions to have
two types of arguments for each, text and regclass.pgstattuple(text)
pgstattuple(regclass)
pgstatindex(text)
pgstatindex(regclass)
pg_relpages(text)
pg_relpages(regclass)I still think a regclass argument is more appropriate for passing
relation/index name to a function than text-type, but having both
arguments in each function seems to be a good choice at this moment,
in terms of backward-compatibility.Docs needs to be updated if this change going to be applied.
Yes, please.
Any comments?
'make installcheck' failed in my machine.
Do we need to remove pgstattuple--1.1.sql and create pgstattuple--1.1--1.2.sql?
+/* contrib/pgstattuple/pgstattuple--1.1.sql */
Typo: s/1.1/1.2
You seem to have forgotten to update pgstattuple.c.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
(2013/07/18 2:31), Fujii Masao wrote:
On Tue, Jul 16, 2013 at 3:00 PM, Satoshi Nagayasu <snaga@uptime.jp> wrote:
(2013/07/04 3:58), Fujii Masao wrote:
For the test, I just implemented the regclass-version of pg_relpages()
(patch attached) and tested some cases. But I could not get that problem.SELECT pg_relpages('hoge'); -- OK
SELECT pg_relpages(oid) FROM pg_class WHERE relname = 'hoge'; -- OK
SELECT pg_relpages(relname) FROM pg_class WHERE relname = 'hoge'; -- OKIn the attached patch, I cleaned up three functions to have
two types of arguments for each, text and regclass.pgstattuple(text)
pgstattuple(regclass)
pgstatindex(text)
pgstatindex(regclass)
pg_relpages(text)
pg_relpages(regclass)I still think a regclass argument is more appropriate for passing
relation/index name to a function than text-type, but having both
arguments in each function seems to be a good choice at this moment,
in terms of backward-compatibility.Docs needs to be updated if this change going to be applied.
Yes, please.
Updated docs and code comments, etc. PFA.
Any comments?
'make installcheck' failed in my machine.
Hmm, it works on my box...
Do we need to remove pgstattuple--1.1.sql and create pgstattuple--1.1--1.2.sql?
+/* contrib/pgstattuple/pgstattuple--1.1.sql */
Typo: s/1.1/1.2
Done.
You seem to have forgotten to update pgstattuple.c.
Should I change something in pgstattuple.c?
I just changed CREATE FUNCTION statement for pgstattuple
to replace oid input arg with the regclass.
Regards,
--
Satoshi Nagayasu <snaga@uptime.jp>
Uptime Technologies, LLC. http://www.uptime.jp
Attachments:
pgstattuple_regclass_v3.difftext/plain; charset=Shift_JIS; name=pgstattuple_regclass_v3.diffDownload
diff --git a/contrib/pgstattuple/Makefile b/contrib/pgstattuple/Makefile
index fc893d8..957742a 100644
--- a/contrib/pgstattuple/Makefile
+++ b/contrib/pgstattuple/Makefile
@@ -4,7 +4,7 @@ MODULE_big = pgstattuple
OBJS = pgstattuple.o pgstatindex.o
EXTENSION = pgstattuple
-DATA = pgstattuple--1.1.sql pgstattuple--1.0--1.1.sql pgstattuple--unpackaged--1.0.sql
+DATA = pgstattuple--1.2.sql pgstattuple--1.0--1.1.sql pgstattuple--unpackaged--1.0.sql
REGRESS = pgstattuple
diff --git a/contrib/pgstattuple/expected/pgstattuple.out b/contrib/pgstattuple/expected/pgstattuple.out
index ab28f50..eaba306 100644
--- a/contrib/pgstattuple/expected/pgstattuple.out
+++ b/contrib/pgstattuple/expected/pgstattuple.out
@@ -11,12 +11,24 @@ select * from pgstattuple('test'::text);
0 | 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0
(1 row)
+select * from pgstattuple('test'::name);
+ table_len | tuple_count | tuple_len | tuple_percent | dead_tuple_count | dead_tuple_len | dead_tuple_percent | free_space | free_percent
+-----------+-------------+-----------+---------------+------------------+----------------+--------------------+------------+--------------
+ 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0
+(1 row)
+
select * from pgstattuple('test'::regclass);
table_len | tuple_count | tuple_len | tuple_percent | dead_tuple_count | dead_tuple_len | dead_tuple_percent | free_space | free_percent
-----------+-------------+-----------+---------------+------------------+----------------+--------------------+------------+--------------
0 | 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0
(1 row)
+select * from pgstattuple('test'::regclass::oid);
+ table_len | tuple_count | tuple_len | tuple_percent | dead_tuple_count | dead_tuple_len | dead_tuple_percent | free_space | free_percent
+-----------+-------------+-----------+---------------+------------------+----------------+--------------------+------------+--------------
+ 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0 | 0
+(1 row)
+
select * from pgstatindex('test_pkey');
version | tree_level | index_size | root_block_no | internal_pages | leaf_pages | empty_pages | deleted_pages | avg_leaf_density | leaf_fragmentation
---------+------------+------------+---------------+----------------+------------+-------------+---------------+------------------+--------------------
diff --git a/contrib/pgstattuple/pgstatindex.c b/contrib/pgstattuple/pgstatindex.c
index 97f897e..41e90e3 100644
--- a/contrib/pgstattuple/pgstatindex.c
+++ b/contrib/pgstattuple/pgstatindex.c
@@ -39,12 +39,24 @@
#include "utils/rel.h"
+/*
+ * Because of backward-compatibility issue, we have decided to have
+ * two types of interfaces, with regclass-type input arg and text-type
+ * input arg, for each function.
+ *
+ * Those functions which have text-type input arg will be depreciated
+ * in the future release.
+ */
extern Datum pgstatindex(PG_FUNCTION_ARGS);
+extern Datum pgstatindexbyid(PG_FUNCTION_ARGS);
extern Datum pg_relpages(PG_FUNCTION_ARGS);
+extern Datum pg_relpagesbyid(PG_FUNCTION_ARGS);
extern Datum pgstatginindex(PG_FUNCTION_ARGS);
PG_FUNCTION_INFO_V1(pgstatindex);
+PG_FUNCTION_INFO_V1(pgstatindexbyid);
PG_FUNCTION_INFO_V1(pg_relpages);
+PG_FUNCTION_INFO_V1(pg_relpagesbyid);
PG_FUNCTION_INFO_V1(pgstatginindex);
#define IS_INDEX(r) ((r)->rd_rel->relkind == RELKIND_INDEX)
@@ -97,6 +109,8 @@ typedef struct GinIndexStat
int64 pending_tuples;
} GinIndexStat;
+static Datum pgstatindex_impl(Relation, FunctionCallInfo);
+
/* ------------------------------------------------------
* pgstatindex()
*
@@ -109,11 +123,6 @@ pgstatindex(PG_FUNCTION_ARGS)
text *relname = PG_GETARG_TEXT_P(0);
Relation rel;
RangeVar *relrv;
- Datum result;
- BlockNumber nblocks;
- BlockNumber blkno;
- BTIndexStat indexStat;
- BufferAccessStrategy bstrategy = GetAccessStrategy(BAS_BULKREAD);
if (!superuser())
ereport(ERROR,
@@ -123,6 +132,34 @@ pgstatindex(PG_FUNCTION_ARGS)
relrv = makeRangeVarFromNameList(textToQualifiedNameList(relname));
rel = relation_openrv(relrv, AccessShareLock);
+ PG_RETURN_DATUM( pgstatindex_impl(rel, fcinfo) );
+}
+
+Datum
+pgstatindexbyid(PG_FUNCTION_ARGS)
+{
+ Oid relid = PG_GETARG_OID(0);
+ Relation rel;
+
+ if (!superuser())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ (errmsg("must be superuser to use pgstattuple functions"))));
+
+ rel = relation_open(relid, AccessShareLock);
+
+ PG_RETURN_DATUM( pgstatindex_impl(rel, fcinfo) );
+}
+
+static Datum
+pgstatindex_impl(Relation rel, FunctionCallInfo fcinfo)
+{
+ Datum result;
+ BlockNumber nblocks;
+ BlockNumber blkno;
+ BTIndexStat indexStat;
+ BufferAccessStrategy bstrategy = GetAccessStrategy(BAS_BULKREAD);
+
if (!IS_INDEX(rel) || !IS_BTREE(rel))
elog(ERROR, "relation \"%s\" is not a btree index",
RelationGetRelationName(rel));
@@ -274,7 +311,7 @@ pgstatindex(PG_FUNCTION_ARGS)
result = HeapTupleGetDatum(tuple);
}
- PG_RETURN_DATUM(result);
+ return result;
}
/* --------------------------------------------------------
@@ -311,6 +348,29 @@ pg_relpages(PG_FUNCTION_ARGS)
PG_RETURN_INT64(relpages);
}
+Datum
+pg_relpagesbyid(PG_FUNCTION_ARGS)
+{
+ Oid relid = PG_GETARG_OID(0);
+ int64 relpages;
+ Relation rel;
+
+ if (!superuser())
+ ereport(ERROR,
+ (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE),
+ (errmsg("must be superuser to use pgstattuple functions"))));
+
+ rel = relation_open(relid, AccessShareLock);
+
+ /* note: this will work OK on non-local temp tables */
+
+ relpages = RelationGetNumberOfBlocks(rel);
+
+ relation_close(rel, AccessShareLock);
+
+ PG_RETURN_INT64(relpages);
+}
+
/* ------------------------------------------------------
* pgstatginindex()
*
diff --git a/contrib/pgstattuple/pgstattuple--1.1--1.2.sql b/contrib/pgstattuple/pgstattuple--1.1--1.2.sql
new file mode 100644
index 0000000..3c9c2a2
--- /dev/null
+++ b/contrib/pgstattuple/pgstattuple--1.1--1.2.sql
@@ -0,0 +1,39 @@
+/* contrib/pgstattuple/pgstattuple--1.1--1.2.sql */
+
+-- complain if script is sourced in psql, rather than via ALTER EXTENSION
+\echo Use "ALTER EXTENSION pgstattuple UPDATE TO '1.2'" to load this file. \quit
+
+DROP FUNCTION pgstattuple(oid);
+
+CREATE FUNCTION pgstattuple(IN reloid regclass,
+ OUT table_len BIGINT, -- physical table length in bytes
+ OUT tuple_count BIGINT, -- number of live tuples
+ OUT tuple_len BIGINT, -- total tuples length in bytes
+ OUT tuple_percent FLOAT8, -- live tuples in %
+ OUT dead_tuple_count BIGINT, -- number of dead tuples
+ OUT dead_tuple_len BIGINT, -- total dead tuples length in bytes
+ OUT dead_tuple_percent FLOAT8, -- dead tuples in %
+ OUT free_space BIGINT, -- free space in bytes
+ OUT free_percent FLOAT8) -- free space in %
+AS 'MODULE_PATHNAME', 'pgstattuplebyid'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION pgstatindex(IN relname regclass,
+ OUT version INT,
+ OUT tree_level INT,
+ OUT index_size BIGINT,
+ OUT root_block_no BIGINT,
+ OUT internal_pages BIGINT,
+ OUT leaf_pages BIGINT,
+ OUT empty_pages BIGINT,
+ OUT deleted_pages BIGINT,
+ OUT avg_leaf_density FLOAT8,
+ OUT leaf_fragmentation FLOAT8)
+AS 'MODULE_PATHNAME', 'pgstatindexbyid'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION pg_relpages(IN relname regclass)
+RETURNS BIGINT
+AS 'MODULE_PATHNAME', 'pg_relpagesbyid'
+LANGUAGE C STRICT;
+
diff --git a/contrib/pgstattuple/pgstattuple--1.1.sql b/contrib/pgstattuple/pgstattuple--1.1.sql
deleted file mode 100644
index b21fbf8..0000000
--- a/contrib/pgstattuple/pgstattuple--1.1.sql
+++ /dev/null
@@ -1,58 +0,0 @@
-/* contrib/pgstattuple/pgstattuple--1.1.sql */
-
--- complain if script is sourced in psql, rather than via CREATE EXTENSION
-\echo Use "CREATE EXTENSION pgstattuple" to load this file. \quit
-
-CREATE FUNCTION pgstattuple(IN relname text,
- OUT table_len BIGINT, -- physical table length in bytes
- OUT tuple_count BIGINT, -- number of live tuples
- OUT tuple_len BIGINT, -- total tuples length in bytes
- OUT tuple_percent FLOAT8, -- live tuples in %
- OUT dead_tuple_count BIGINT, -- number of dead tuples
- OUT dead_tuple_len BIGINT, -- total dead tuples length in bytes
- OUT dead_tuple_percent FLOAT8, -- dead tuples in %
- OUT free_space BIGINT, -- free space in bytes
- OUT free_percent FLOAT8) -- free space in %
-AS 'MODULE_PATHNAME', 'pgstattuple'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION pgstattuple(IN reloid oid,
- OUT table_len BIGINT, -- physical table length in bytes
- OUT tuple_count BIGINT, -- number of live tuples
- OUT tuple_len BIGINT, -- total tuples length in bytes
- OUT tuple_percent FLOAT8, -- live tuples in %
- OUT dead_tuple_count BIGINT, -- number of dead tuples
- OUT dead_tuple_len BIGINT, -- total dead tuples length in bytes
- OUT dead_tuple_percent FLOAT8, -- dead tuples in %
- OUT free_space BIGINT, -- free space in bytes
- OUT free_percent FLOAT8) -- free space in %
-AS 'MODULE_PATHNAME', 'pgstattuplebyid'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION pgstatindex(IN relname text,
- OUT version INT,
- OUT tree_level INT,
- OUT index_size BIGINT,
- OUT root_block_no BIGINT,
- OUT internal_pages BIGINT,
- OUT leaf_pages BIGINT,
- OUT empty_pages BIGINT,
- OUT deleted_pages BIGINT,
- OUT avg_leaf_density FLOAT8,
- OUT leaf_fragmentation FLOAT8)
-AS 'MODULE_PATHNAME', 'pgstatindex'
-LANGUAGE C STRICT;
-
-CREATE FUNCTION pg_relpages(IN relname text)
-RETURNS BIGINT
-AS 'MODULE_PATHNAME', 'pg_relpages'
-LANGUAGE C STRICT;
-
-/* New stuff in 1.1 begins here */
-
-CREATE FUNCTION pgstatginindex(IN relname regclass,
- OUT version INT4,
- OUT pending_pages INT4,
- OUT pending_tuples BIGINT)
-AS 'MODULE_PATHNAME', 'pgstatginindex'
-LANGUAGE C STRICT;
diff --git a/contrib/pgstattuple/pgstattuple--1.2.sql b/contrib/pgstattuple/pgstattuple--1.2.sql
new file mode 100644
index 0000000..7477d0c
--- /dev/null
+++ b/contrib/pgstattuple/pgstattuple--1.2.sql
@@ -0,0 +1,77 @@
+/* contrib/pgstattuple/pgstattuple--1.2.sql */
+
+-- complain if script is sourced in psql, rather than via CREATE EXTENSION
+\echo Use "CREATE EXTENSION pgstattuple" to load this file. \quit
+
+CREATE FUNCTION pgstattuple(IN relname text,
+ OUT table_len BIGINT, -- physical table length in bytes
+ OUT tuple_count BIGINT, -- number of live tuples
+ OUT tuple_len BIGINT, -- total tuples length in bytes
+ OUT tuple_percent FLOAT8, -- live tuples in %
+ OUT dead_tuple_count BIGINT, -- number of dead tuples
+ OUT dead_tuple_len BIGINT, -- total dead tuples length in bytes
+ OUT dead_tuple_percent FLOAT8, -- dead tuples in %
+ OUT free_space BIGINT, -- free space in bytes
+ OUT free_percent FLOAT8) -- free space in %
+AS 'MODULE_PATHNAME', 'pgstattuple'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION pgstattuple(IN reloid regclass,
+ OUT table_len BIGINT, -- physical table length in bytes
+ OUT tuple_count BIGINT, -- number of live tuples
+ OUT tuple_len BIGINT, -- total tuples length in bytes
+ OUT tuple_percent FLOAT8, -- live tuples in %
+ OUT dead_tuple_count BIGINT, -- number of dead tuples
+ OUT dead_tuple_len BIGINT, -- total dead tuples length in bytes
+ OUT dead_tuple_percent FLOAT8, -- dead tuples in %
+ OUT free_space BIGINT, -- free space in bytes
+ OUT free_percent FLOAT8) -- free space in %
+AS 'MODULE_PATHNAME', 'pgstattuplebyid'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION pgstatindex(IN relname text,
+ OUT version INT,
+ OUT tree_level INT,
+ OUT index_size BIGINT,
+ OUT root_block_no BIGINT,
+ OUT internal_pages BIGINT,
+ OUT leaf_pages BIGINT,
+ OUT empty_pages BIGINT,
+ OUT deleted_pages BIGINT,
+ OUT avg_leaf_density FLOAT8,
+ OUT leaf_fragmentation FLOAT8)
+AS 'MODULE_PATHNAME', 'pgstatindex'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION pgstatindex(IN relname regclass,
+ OUT version INT,
+ OUT tree_level INT,
+ OUT index_size BIGINT,
+ OUT root_block_no BIGINT,
+ OUT internal_pages BIGINT,
+ OUT leaf_pages BIGINT,
+ OUT empty_pages BIGINT,
+ OUT deleted_pages BIGINT,
+ OUT avg_leaf_density FLOAT8,
+ OUT leaf_fragmentation FLOAT8)
+AS 'MODULE_PATHNAME', 'pgstatindexbyid'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION pg_relpages(IN relname text)
+RETURNS BIGINT
+AS 'MODULE_PATHNAME', 'pg_relpages'
+LANGUAGE C STRICT;
+
+CREATE FUNCTION pg_relpages(IN relname regclass)
+RETURNS BIGINT
+AS 'MODULE_PATHNAME', 'pg_relpagesbyid'
+LANGUAGE C STRICT;
+
+/* New stuff in 1.1 begins here */
+
+CREATE FUNCTION pgstatginindex(IN relname regclass,
+ OUT version INT4,
+ OUT pending_pages INT4,
+ OUT pending_tuples BIGINT)
+AS 'MODULE_PATHNAME', 'pgstatginindex'
+LANGUAGE C STRICT;
diff --git a/contrib/pgstattuple/pgstattuple.control b/contrib/pgstattuple/pgstattuple.control
index fcfd36f..a7cf47f 100644
--- a/contrib/pgstattuple/pgstattuple.control
+++ b/contrib/pgstattuple/pgstattuple.control
@@ -1,5 +1,5 @@
# pgstattuple extension
comment = 'show tuple-level statistics'
-default_version = '1.1'
+default_version = '1.2'
module_pathname = '$libdir/pgstattuple'
relocatable = true
diff --git a/contrib/pgstattuple/sql/pgstattuple.sql b/contrib/pgstattuple/sql/pgstattuple.sql
index 8cb350d..76369c5 100644
--- a/contrib/pgstattuple/sql/pgstattuple.sql
+++ b/contrib/pgstattuple/sql/pgstattuple.sql
@@ -9,12 +9,23 @@ CREATE EXTENSION pgstattuple;
create table test (a int primary key, b int[]);
select * from pgstattuple('test'::text);
+select * from pgstattuple('test'::name);
select * from pgstattuple('test'::regclass);
+select * from pgstattuple('test'::regclass::oid);
+select pgstattuple(relname) from pg_class where relname = 'test' and relkind = 'r';
-select * from pgstatindex('test_pkey');
+select * from pgstatindex('test_pkey'::text);
+select * from pgstatindex('test_pkey'::name);
+select * from pgstatindex('test_pkey'::regclass);
+select * from pgstatindex('test_pkey'::regclass::oid);
+select pgstatindex(relname) from pg_class where relname = 'test_pkey' and relkind = 'i';
select pg_relpages('test');
select pg_relpages('test_pkey');
+select pg_relpages('test_pkey'::name);
+select pg_relpages('test_pkey'::regclass);
+select pg_relpages('test_pkey'::regclass::oid);
+select pg_relpages(relname) from pg_class where relname = 'test_pkey' and relkind = 'i';
create index test_ginidx on test using gin (b);
diff --git a/doc/src/sgml/pgstattuple.sgml b/doc/src/sgml/pgstattuple.sgml
index f2bc2a6..64448f5 100644
--- a/doc/src/sgml/pgstattuple.sgml
+++ b/doc/src/sgml/pgstattuple.sgml
@@ -22,7 +22,7 @@
</indexterm>
<term>
- <function>pgstattuple(text) returns record</>
+ <function>pgstattuple(regclass) returns record</>
</term>
<listitem>
@@ -30,7 +30,7 @@
<function>pgstattuple</function> returns a relation's physical length,
percentage of <quote>dead</> tuples, and other info. This may help users
to determine whether vacuum is necessary or not. The argument is the
- target relation's name (optionally schema-qualified).
+ target relation's name (optionally schema-qualified) or OID.
For example:
<programlisting>
test=> SELECT * FROM pgstattuple('pg_catalog.pg_proc');
@@ -125,13 +125,15 @@ free_percent | 1.95
<varlistentry>
<term>
- <function>pgstattuple(oid) returns record</>
+ <function>pgstattuple(text) returns record</>
</term>
<listitem>
<para>
- This is the same as <function>pgstattuple(text)</function>, except
- that the target relation is specified by OID.
+ This is the same as <function>pgstattuple(regclass)</function>, except
+ that the target relation is specified by TEXT. This function is kept
+ because of backward-compatibility so far, and will be depreciated in
+ the future release.
</para>
</listitem>
</varlistentry>
@@ -141,7 +143,7 @@ free_percent | 1.95
<indexterm>
<primary>pgstatindex</primary>
</indexterm>
- <function>pgstatindex(text) returns record</>
+ <function>pgstatindex(regclass) returns record</>
</term>
<listitem>
@@ -253,6 +255,21 @@ leaf_fragmentation | 0
<varlistentry>
<term>
+ <function>pgstatindex(text) returns record</>
+ </term>
+
+ <listitem>
+ <para>
+ This is the same as <function>pgstatindex(regclass)</function>, except
+ that the target index is specified by TEXT. This function is kept
+ because of backward-compatibility so far, and will be depreciated in
+ the future release.
+ </para>
+ </listitem>
+ </varlistentry>
+
+ <varlistentry>
+ <term>
<indexterm>
<primary>pgstatginindex</primary>
</indexterm>
@@ -316,7 +333,7 @@ pending_tuples | 0
<indexterm>
<primary>pg_relpages</primary>
</indexterm>
- <function>pg_relpages(text) returns bigint</>
+ <function>pg_relpages(regclass) returns bigint</>
</term>
<listitem>
@@ -326,6 +343,22 @@ pending_tuples | 0
</para>
</listitem>
</varlistentry>
+
+ <varlistentry>
+ <term>
+ <function>pg_relpages(text) returns bigint</>
+ </term>
+
+ <listitem>
+ <para>
+ This is the same as <function>pg_relpages(regclass)</function>, except
+ that the target relation is specified by TEXT. This function is kept
+ because of backward-compatibility so far, and will be depreciated in
+ the future release.
+ </para>
+ </listitem>
+ </varlistentry>
+
</variablelist>
</sect2>
On Thu, Jul 18, 2013 at 9:40 AM, Satoshi Nagayasu <snaga@uptime.jp> wrote:
(2013/07/18 2:31), Fujii Masao wrote:
On Tue, Jul 16, 2013 at 3:00 PM, Satoshi Nagayasu <snaga@uptime.jp>
wrote:(2013/07/04 3:58), Fujii Masao wrote:
For the test, I just implemented the regclass-version of pg_relpages()
(patch attached) and tested some cases. But I could not get that
problem.SELECT pg_relpages('hoge'); -- OK
SELECT pg_relpages(oid) FROM pg_class WHERE relname = 'hoge';
-- OK
SELECT pg_relpages(relname) FROM pg_class WHERE relname = 'hoge';
-- OKIn the attached patch, I cleaned up three functions to have
two types of arguments for each, text and regclass.pgstattuple(text)
pgstattuple(regclass)
pgstatindex(text)
pgstatindex(regclass)
pg_relpages(text)
pg_relpages(regclass)I still think a regclass argument is more appropriate for passing
relation/index name to a function than text-type, but having both
arguments in each function seems to be a good choice at this moment,
in terms of backward-compatibility.Docs needs to be updated if this change going to be applied.
Yes, please.
Updated docs and code comments, etc. PFA.
Looks good.
Any comments?
'make installcheck' failed in my machine.
Hmm, it works on my box...
Works for me too.
Overall looks good to me.
Do we need to remove pgstattuple--1.1.sql and create
pgstattuple--1.1--1.2.sql?
+/* contrib/pgstattuple/**pgstattuple--1.1.sql */
Typo: s/1.1/1.2
Done.
You seem to have forgotten to update pgstattuple.c.
Should I change something in pgstattuple.c?
I just changed CREATE FUNCTION statement for pgstattuple
to replace oid input arg with the regclass.Regards,
--
Satoshi Nagayasu <snaga@uptime.jp>
Uptime Technologies, LLC. http://www.uptime.jp--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Thanks,
Rushabh Lathia
On Thu, Jul 18, 2013 at 1:49 PM, Rushabh Lathia
<rushabh.lathia@gmail.com> wrote:
On Thu, Jul 18, 2013 at 9:40 AM, Satoshi Nagayasu <snaga@uptime.jp> wrote:
(2013/07/18 2:31), Fujii Masao wrote:
On Tue, Jul 16, 2013 at 3:00 PM, Satoshi Nagayasu <snaga@uptime.jp>
wrote:(2013/07/04 3:58), Fujii Masao wrote:
For the test, I just implemented the regclass-version of pg_relpages()
(patch attached) and tested some cases. But I could not get that
problem.SELECT pg_relpages('hoge'); -- OK
SELECT pg_relpages(oid) FROM pg_class WHERE relname = 'hoge';
-- OK
SELECT pg_relpages(relname) FROM pg_class WHERE relname = 'hoge';
-- OKIn the attached patch, I cleaned up three functions to have
two types of arguments for each, text and regclass.pgstattuple(text)
pgstattuple(regclass)
pgstatindex(text)
pgstatindex(regclass)
pg_relpages(text)
pg_relpages(regclass)I still think a regclass argument is more appropriate for passing
relation/index name to a function than text-type, but having both
arguments in each function seems to be a good choice at this moment,
in terms of backward-compatibility.Docs needs to be updated if this change going to be applied.
Yes, please.
Updated docs and code comments, etc. PFA.
Thanks for updating the patch. Committed.
'make installcheck' failed in my machine.
Hmm, it works on my box...
Works for me too.
Hmm... make installcheck still failed on my box. That's because
you added several SELECT queries into sql/pgstattuple.sql, but
you just added only two results into expected/pgstattuple.out.
I corrected the regression test code of pgstattuple.
Regards,
--
Fujii Masao
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers