Re: Fwd: Proposal: variant of regclass

On Wed, Feb 26, 2014 at 11:56 AM, Yugo Nagata <nagata(at)sraoss(dot)co(dot)jp> wrote:
> Thanks for your a lot of comments. I revised the patch according to
> comments from Robert Haas and Marti Raudsepp.

I have started looking into this patch and below are my
initial findings:

1. Dependency is not recorded when to_regclass is used,
however it is recorded when ::regclass is used. Below is
test to demonstrate this point. This change in behaviour is
neither discussed nor mentioned in docs along with patch.

usage of ::regclass

create sequence my_seq;
create table t1 (c1 int, c2 int default 'my_seq'::regclass);
insert into t1 values(1);
insert into t1 values(2);
select * from t1;
c1 | c2
----+-------
1 | 16390
2 | 16390
(2 rows)

drop sequence my_seq;
ERROR: cannot drop sequence my_seq because other
objects depend on it
DETAIL: default for table t1 column c2 depends on
sequence my_seq
HINT: Use DROP ... CASCADE to drop the dependent
objects too.

Check pg_depend has entry for dependency of default
value of table-column on seq.

postgres=# select * from pg_depend where refobjid = 16390;
classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
---------+-------+----------+------------+----------+-------------+---------
1247 | 16391 | 0 | 1259 | 16390 | 0 | i
2604 | 16395 | 0 | 1259 | 16390 | 0 | n
(2 rows)

postgres=# select oid,adrelid from pg_attrdef;
oid | adrelid
-------+---------
16395 | 16392
(1 row)

usage of to_regclass

create sequence my_seq;
create table t1 (c1 int, c2 int default to_regclass('my_seq'));
insert into t1 values(1);
insert into t1 values(2);
select * from t1;
c1 | c2
----+-------
1 | 16396
2 | 16396

select * from pg_depend where refobjid = 16396;
classid | objid | objsubid | refclassid | refobjid | refobjsubid | deptype
---------+-------+----------+------------+----------+-------------+---------
1247 | 16397 | 0 | 1259 | 16396 | 0 | i
(1 row)

There is only one entry for pg_type which means the
dependent object on sequence is only its type, so it will
allow to drop sequence.

postgres=# drop sequence my_seq;
DROP SEQUENCE

2.
! if (!((Form_pg_type) GETSTRUCT(tup))->typisdefined)
! ereport(ERROR,
! (errcode(ERRCODE_UNDEFINED_OBJECT),
! errmsg("type \"%s\" is only a shell",
! TypeNameToString(typeName)),
! parser_errposition(NULL, typeName->location)));

In this case it is not exactly same as object does not exist
so I think we should not avoid error in this case
and infact OID is valid for such a case, but in else part
you have assigned it as InvalidOid which seems to be wrong.

3.
regproc_guts()
! {
! if (!missing_ok)
! ereport(ERROR,
! (errcode(ERRCODE_AMBIGUOUS_FUNCTION),
! errmsg("more than one function named \"%s\"",
! pro_name_or_oid)));
! return false;
! }

In to_regproc(), this patch is trying to supress error other
"object-name-not-found" which as far as I could read the thread
was not the original idea and even the description in docs
says only about "object-name-not-found" case.
Doc Description
+ similar to the regclass, regproc, regoper and regtype casts, except
+ that they return NULL when the object is not found, instead of raising
+ an error.

Even if you want to avoid the other error('s) like
above, then I think it's better to mention the same in docs.

I am bit worried, how is user going to distinguish between
the cases when object-not-found and more-than-one-object.

I think such a problem would not arise if we write a function
for regprocedure rather than for regproc, also manual says regprocedure
is more appropriate for most usecases.
http://www.postgresql.org/docs/devel/static/datatype-oid.html

Also I think one other advantage for doing so is that we
don't need to pass missing_ok paramter to some of the functions
like FuncnameGetCandidates(), OpernameGetCandidates().

I understand that you might be using regproc/regoper or might have
a usecase for it, but is it possible for you to use regprocedure/regoperator
instead of regproc/regoper?

4.
+ <entry><type>regclass</type></entry>
+ <entry>get the table oid</entry>

Shouldn't it be better to describe it as "get the relation oid" as
it can give oid for other objects like index as well.

5.
+ <para>
+ to_regclass, to_regproc, to_regoper and to_regtype are functions
+ similar to the regclass, regproc, regoper and regtype casts, except

In above description, it is better to use 'object identifier types'
rather than 'casts'.

6.
! * Guts of regoperin and to_regproc.
Here it should be regprocin

7.
* If not found and missing_ok is true, returns false instead of raising
an error.

Above line is more than 80 chars, it should be less than
80 char limit. This needs to be corrected whereever this line is used.

8.
! * Guts of regtypein and to_regtype.
! * If the classname is found, returns true and the OID is stored into
*typid_p.

typo in *If the classname is found*, it should be type is found.

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com