Re: [HACKERS] Fix pgstattuple/pgstatindex to use regclass-type as the argument
On Thu, Jul 18, 2013 at 1:49 PM, Rushabh Lathia
wrote:
>
>
>
> On Thu, Jul 18, 2013 at 9:40 AM, Satoshi Nagayasu wrote:
>>
>> (2013/07/18 2:31), Fujii Masao wrote:
>>>
>>> On Tue, Jul 16, 2013 at 3:00 PM, Satoshi Nagayasu
>>> 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';
> -- 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.
>>>
>>>
>>> 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
Re: [HACKERS] Fix pgstattuple/pgstatindex to use regclass-type as the argument
On Thu, Jul 18, 2013 at 9:40 AM, Satoshi Nagayasu wrote:
> (2013/07/18 2:31), Fujii Masao wrote:
>
>> On Tue, Jul 16, 2013 at 3:00 PM, Satoshi Nagayasu
>> 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';
-- 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.
>>>
>>
>> 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
> 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
Re: [HACKERS] Fix pgstattuple/pgstatindex to use regclass-type as the argument
(2013/07/18 2:31), Fujii Masao wrote:
On Tue, Jul 16, 2013 at 3:00 PM, Satoshi Nagayasu 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';-- 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.
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
Uptime Technologies, LLC. http://www.uptime.jp
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_relpa
Re: [HACKERS] Fix pgstattuple/pgstatindex to use regclass-type as the argument
On Tue, Jul 16, 2013 at 3:00 PM, Satoshi Nagayasu wrote:
> (2013/07/04 3:58), Fujii Masao wrote:
>> On Wed, Jun 26, 2013 at 12:39 AM, Robert Haas wrote:
>>> On Thu, Jun 20, 2013 at 2:32 PM, Fujii Masao 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.
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
Re: [HACKERS] Fix pgstattuple/pgstatindex to use regclass-type as the argument
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 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 > Uptime Technologies, LLC. http://www.uptime.jp > -- Rushabh Lathia
Re: [HACKERS] Fix pgstattuple/pgstatindex to use regclass-type as the argument
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 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
Re: [HACKERS] Fix pgstattuple/pgstatindex to use regclass-type as the argument
(2013/07/04 3:58), Fujii Masao wrote:
> On Wed, Jun 26, 2013 at 12:39 AM, Robert Haas wrote:
>> On Thu, Jun 20, 2013 at 2:32 PM, Fujii Masao 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
Uptime Technologies, LLC. http://www.uptime.jp
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)
Re: [HACKERS] Fix pgstattuple/pgstatindex to use regclass-type as the argument
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 wrote:
> On Wed, Jun 26, 2013 at 12:39 AM, Robert Haas
> wrote:
> > On Thu, Jun 20, 2013 at 2:32 PM, Fujii Masao
> wrote:
> >> On Thu, Jun 20, 2013 at 11:43 AM, Satoshi Nagayasu
> wrote:
> >>> (2013/06/17 4:02), Fujii Masao wrote:
>
> On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu
> 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
>
>
> --
> 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
Re: [HACKERS] Fix pgstattuple/pgstatindex to use regclass-type as the argument
On Wed, Jun 26, 2013 at 12:39 AM, Robert Haas wrote:
> On Thu, Jun 20, 2013 at 2:32 PM, Fujii Masao wrote:
>> On Thu, Jun 20, 2013 at 11:43 AM, Satoshi Nagayasu wrote:
>>> (2013/06/17 4:02), Fujii Masao wrote:
On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu 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
pg_relpages_test.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Fix pgstattuple/pgstatindex to use regclass-type as the argument
On Thu, Jun 20, 2013 at 2:32 PM, Fujii Masao wrote:
> On Thu, Jun 20, 2013 at 11:43 AM, Satoshi Nagayasu wrote:
>> (2013/06/17 4:02), Fujii Masao wrote:
>>>
>>> On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu 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
Re: [HACKERS] Fix pgstattuple/pgstatindex to use regclass-type as the argument
On Thu, Jun 20, 2013 at 11:43 AM, Satoshi Nagayasu wrote:
> (2013/06/17 4:02), Fujii Masao wrote:
>>
>> On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu 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
Re: [HACKERS] Fix pgstattuple/pgstatindex to use regclass-type as the argument
(2013/06/17 4:02), Fujii Masao wrote:
On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu 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
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
Re: [HACKERS] Fix pgstattuple/pgstatindex to use regclass-type as the argument
On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu wrote: > (2013/03/05 22:46), Robert Haas wrote: >> >> On Sun, Mar 3, 2013 at 5:32 PM, Tom Lane 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
Re: [HACKERS] Fix pgstattuple/pgstatindex to use regclass-type as the argument
(2013/03/05 22:46), Robert Haas wrote: On Sun, Mar 3, 2013 at 5:32 PM, Tom Lane 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 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
Re: [HACKERS] Fix pgstattuple/pgstatindex to use regclass-type as the argument
On Sun, Mar 3, 2013 at 5:32 PM, Tom Lane 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
Re: [HACKERS] Fix pgstattuple/pgstatindex to use regclass-type as the argument
Satoshi Nagayasu 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
