Re: 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
http://www.postgresql.org/message-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(at)uptime(dot)jp>
Uptime Technologies, LLC. http://www.uptime.jp

Satoshi Nagayasu <snaga(at)uptime(dot)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

On Sun, Mar 3, 2013 at 5:32 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

(2013/03/05 22:46), Robert Haas wrote:
> On Sun, Mar 3, 2013 at 5:32 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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(at)uptime(dot)jp>
Uptime Technologies, LLC. http://www.uptime.jp

On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu <snaga(at)uptime(dot)jp> wrote:
> (2013/03/05 22:46), Robert Haas wrote:
>>
>> On Sun, Mar 3, 2013 at 5:32 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)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

(2013/06/17 4:02), Fujii Masao wrote:
> On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu <snaga(at)uptime(dot)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(at)uptime(dot)jp>
Uptime Technologies, LLC. http://www.uptime.jp

On Thu, Jun 20, 2013 at 11:43 AM, Satoshi Nagayasu <snaga(at)uptime(dot)jp> wrote:
> (2013/06/17 4:02), Fujii Masao wrote:
>>
>> On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu <snaga(at)uptime(dot)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

On Thu, Jun 20, 2013 at 2:32 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
> On Thu, Jun 20, 2013 at 11:43 AM, Satoshi Nagayasu <snaga(at)uptime(dot)jp> wrote:
>> (2013/06/17 4:02), Fujii Masao wrote:
>>>
>>> On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu <snaga(at)uptime(dot)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

On Wed, Jun 26, 2013 at 12:39 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Thu, Jun 20, 2013 at 2:32 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com> wrote:
>> On Thu, Jun 20, 2013 at 11:43 AM, Satoshi Nagayasu <snaga(at)uptime(dot)jp> wrote:
>>> (2013/06/17 4:02), Fujii Masao wrote:
>>>>
>>>> On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu <snaga(at)uptime(dot)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

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(dot)fujii(at)gmail(dot)com> wrote:

> On Wed, Jun 26, 2013 at 12:39 AM, Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
> > On Thu, Jun 20, 2013 at 2:32 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
> wrote:
> >> On Thu, Jun 20, 2013 at 11:43 AM, Satoshi Nagayasu <snaga(at)uptime(dot)jp>
> wrote:
> >>> (2013/06/17 4:02), Fujii Masao wrote:
> >>>>
> >>>> On Sat, Mar 9, 2013 at 3:23 PM, Satoshi Nagayasu <snaga(at)uptime(dot)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
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)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(at)gmail(dot)com> wrote:
>> On Thu, Jun 20, 2013 at 2:32 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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(at)uptime(dot)jp>
Uptime Technologies, LLC. http://www.uptime.jp

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(at)uptime(dot)jp>
Uptime Technologies, LLC. http://www.uptime.jp

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(at)uptime(dot)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(at)uptime(dot)jp>
> Uptime Technologies, LLC. http://www.uptime.jp
>

--
Rushabh Lathia

On Tue, Jul 16, 2013 at 3:00 PM, Satoshi Nagayasu <snaga(at)uptime(dot)jp> wrote:
> (2013/07/04 3:58), Fujii Masao wrote:
>> On Wed, Jun 26, 2013 at 12:39 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Thu, Jun 20, 2013 at 2:32 PM, Fujii Masao <masao(dot)fujii(at)gmail(dot)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.

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

(2013/07/18 2:31), Fujii Masao wrote:
> On Tue, Jul 16, 2013 at 3:00 PM, Satoshi Nagayasu <snaga(at)uptime(dot)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'; -- 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 <snaga(at)uptime(dot)jp>
Uptime Technologies, LLC. http://www.uptime.jp

On Thu, Jul 18, 2013 at 9:40 AM, Satoshi Nagayasu <snaga(at)uptime(dot)jp> wrote:

> (2013/07/18 2:31), Fujii Masao wrote:
>
>> On Tue, Jul 16, 2013 at 3:00 PM, Satoshi Nagayasu <snaga(at)uptime(dot)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';
>>>> -- 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 <snaga(at)uptime(dot)jp>
> Uptime Technologies, LLC. http://www.uptime.jp
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)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(dot)lathia(at)gmail(dot)com> wrote:
>
>
>
> On Thu, Jul 18, 2013 at 9:40 AM, Satoshi Nagayasu <snaga(at)uptime(dot)jp> wrote:
>>
>> (2013/07/18 2:31), Fujii Masao wrote:
>>>
>>> On Tue, Jul 16, 2013 at 3:00 PM, Satoshi Nagayasu <snaga(at)uptime(dot)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';
>>>>> -- 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