On Thu, Sep 19, 2013 at 10:28 PM, Robert Haas <robertmh...@gmail.com> wrote: >>> Marking this as Ready for committer. >> >> Thanks a ton for reviewing the patch. > > I rewrote the comments for this patch and fixed the incorrect > formatting in parse_relation.c. It used spaces instead of tabs, which > is why if you look at the patch file you'll see that the alignment > looked off. See new version, attached. Thanks, Attached version looks better.
> However, I have a few residual questions: > > 1. Does copy.c also need to be fixed? I see that it also calls > ExecConstraints(), and I don't see it setting tableOid anywhere. We > certainly want COPY and INSERT to behave the same way. I have missed it by confusing it with OID, as OID is set in COPY. I think along with COPY, it needs to set in ATRewriteTable() as well, because during rewrite also we check constraints. I will send an updated patch after point-2 is concluded. > > 2. Should constraints also allow access to the "oid" column? Right > now you can do that but it doesn't seem to work, e.g.: > > rhaas=# create table foo (a int, check (oid::integer % 2 = 0)) with oids; > CREATE TABLE I have tried various combinations, it is giving error at my end. postgres=# create table tbl_with_oid(c1 int) With OIDS; CREATE TABLE postgres=# alter table tbl_with_oid add check (oid < 16403); ERROR: system column "oid" reference in check constraint is invalid postgres=# alter table tbl_with_oid add check (oid =0); ERROR: system column "oid" reference in check constraint is invalid postgres=# alter table tbl_with_oid add check (oid::integer %2 =0); ERROR: system column "oid" reference in check constraint is invalid postgres=# create table foo (a int, check (oid::integer % 2 = 0)) with oids; ERROR: system column "oid" reference in check constraint is invalid > Just prohibiting that might be fine; it doesn't seem that useful > anyway. Currently only tableOid is allowed, other system columns should error out due to below check: + /* In constraint check, no system column is allowed except tableOid */ + if (pstate->p_expr_kind == EXPR_KIND_CHECK_CONSTRAINT && + attnum < InvalidAttrNumber && attnum != TableOidAttributeNumber) > But if we're setting the table OID, maybe we should set the > OID too, and then just allow this. It can be done for OID as well. I don't have any strong reason for either doing or not doing it, if you think it can be of use then we can add it. With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
