Обсуждение: BUG #15833: defining a comment on a domain constraint fails with wrong OID

The following bug has been logged on the website:

Bug reference:      15833
Logged by:          Clemens Ladisch
Email address:      clemens@ladisch.de
PostgreSQL version: 10.8
Operating system:   Windows 64-bit
Description:

postgres=> CREATE DOMAIN ddd AS text CONSTRAINT ccc CHECK (TRUE);
CREATE DOMAIN
postgres=> COMMENT ON CONSTRAINT ccc ON DOMAIN ddd IS 'test';
ERROR:  42704: type with OID 444275 does not exist
LOCATION:  pg_type_ownercheck, aclchk.c:4585

And there is indeed no type with that OID:

postgres=> SELECT 'type', oid FROM pg_type WHERE typname = 'ddd' UNION ALL
SELECT 'constraint', oid FROM pg_constraint WHERE conname = 'ccc';
  ?column?  |  oid
------------+--------
 type       | 444274
 constraint | 444275
(2 rows)

Same with Postgres 9.6 on SQLFiddle:
<http://www.sqlfiddle.com/#!17/a63b6/2>

On 2019-Jun-05, PG Bug reporting form wrote:

> postgres=> CREATE DOMAIN ddd AS text CONSTRAINT ccc CHECK (TRUE);
> CREATE DOMAIN
> postgres=> COMMENT ON CONSTRAINT ccc ON DOMAIN ddd IS 'test';
> ERROR:  42704: type with OID 444275 does not exist
> LOCATION:  pg_type_ownercheck, aclchk.c:4585

Confirmed.  It works for superusers, which explains why the existing
regression tests pass -- and that's because check_object_ownership()
(which is handing the OBJECT_DOMCONSTRAINT case wrongly) is bypassed for
superusers.  Annoyingly, get_object_address does not return the type's
OID, only the domain's.

I'm surprised that this has been broken for so long with no reports ...
I broke it in 7eca575d1c28 (December 2014).

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Wed, Jun 05, 2019 at 02:15:02PM -0400, Alvaro Herrera wrote:
> Confirmed.  It works for superusers, which explains why the existing
> regression tests pass -- and that's because check_object_ownership()
> (which is handing the OBJECT_DOMCONSTRAINT case wrongly) is bypassed for
> superusers.  Annoyingly, get_object_address does not return the type's
> OID, only the domain's.

Well, it wouldn't be a problem to do a syscache lookup and then use
the type from contypid, no?  It seems to me that it would be more
consistent to just add a pg_domain_constraint_ownercheck() in aclchk.c
as all the syscache lookups happen their for all the other objects
types.  What do you think?
--
Michael

On Fri, Jun 07, 2019 at 02:42:33PM +0900, Michael Paquier wrote:
> Well, it wouldn't be a problem to do a syscache lookup and then use
> the type from contypid, no?  It seems to me that it would be more
> consistent to just add a pg_domain_constraint_ownercheck() in aclchk.c
> as all the syscache lookups happen their for all the other objects
> types.  What do you think?

Attached is what I have in mind.  There are already tests at the
bottom of constraints.source checking for comments on both table and
domain constraints, so my proposal is to run them with a dedicated
role.  What do you think?
--
Michael

> On 10 Jun 2019, at 08:28, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Jun 07, 2019 at 02:42:33PM +0900, Michael Paquier wrote:
>> Well, it wouldn't be a problem to do a syscache lookup and then use
>> the type from contypid, no?  It seems to me that it would be more
>> consistent to just add a pg_domain_constraint_ownercheck() in aclchk.c
>> as all the syscache lookups happen their for all the other objects
>> types.  What do you think?
>
> Attached is what I have in mind.  There are already tests at the
> bottom of constraints.source checking for comments on both table and
> domain constraints, so my proposal is to run them with a dedicated
> role.  What do you think?

+1 on the approach of the patch, it seems like the simplest approach.  A
comment on the check_object_ownership() diff though:

+    if (!pg_domain_constraint_ownercheck(address.objectId, roleid))
+        aclcheck_error_type(ACLCHECK_NOT_OWNER, address.objectId);

This doesn’t work for the errorcase as the address.objectId is the wrong Oid
here as well, the contypid extracted in pg_domain_constraint_ownercheck() is
required.  I’ve hacked up your patch to pass it back and that seems to work,
and also added a test for the errorpath.  Another option would be to provide a
new aclcheck_error_domain_constraint(), not sure which is best.

cheers ./daniel

On 2019-Jun-10, Daniel Gustafsson wrote:

> +1 on the approach of the patch, it seems like the simplest approach.  A
> comment on the check_object_ownership() diff though:
> 
> +    if (!pg_domain_constraint_ownercheck(address.objectId, roleid))
> +        aclcheck_error_type(ACLCHECK_NOT_OWNER, address.objectId);
> 
> This doesn’t work for the errorcase as the address.objectId is the wrong Oid
> here as well, the contypid extracted in pg_domain_constraint_ownercheck() is
> required.  I’ve hacked up your patch to pass it back and that seems to work,

-1 on this approach.  Having this ownercheck function return the owning
object ID seems way too strange.  I'd rather not have the new ownercheck
function, and instead do a syscache search to obtain the type OID in
check_object_ownership, then do pg_type_ownercheck.  I'm not even sure
that pg_domain_constraint_ownercheck makes a lot of sense in itself,
since it's never the constraint that requires an owner check.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Mon, Jun 10, 2019 at 08:55:27AM -0400, Alvaro Herrera wrote:
> -1 on this approach.  Having this ownercheck function return the owning
> object ID seems way too strange.  I'd rather not have the new ownercheck
> function, and instead do a syscache search to obtain the type OID in
> check_object_ownership, then do pg_type_ownercheck.  I'm not even sure
> that pg_domain_constraint_ownercheck makes a lot of sense in itself,
> since it's never the constraint that requires an owner check.

I can see your point, yes perhaps I overdid it.  What do you think
about the attached instead?  This moves the syscache lookup directly
into check_object_ownership() as you suggest.
--
Michael

On 2019-Jun-11, Michael Paquier wrote:

> On Mon, Jun 10, 2019 at 08:55:27AM -0400, Alvaro Herrera wrote:
> > -1 on this approach.  Having this ownercheck function return the owning
> > object ID seems way too strange.  I'd rather not have the new ownercheck
> > function, and instead do a syscache search to obtain the type OID in
> > check_object_ownership, then do pg_type_ownercheck.  I'm not even sure
> > that pg_domain_constraint_ownercheck makes a lot of sense in itself,
> > since it's never the constraint that requires an owner check.
> 
> I can see your point, yes perhaps I overdid it.  What do you think
> about the attached instead?  This moves the syscache lookup directly
> into check_object_ownership() as you suggest.

Yeah, looks better.  I think the error message should be a normal elog()
cache failure, though ... at least in the COMMENT case, the obj-does-not-
exist message is supposed to be thrown by get_object_address(), before
check_object_ownership is called.

As a matter of style, I would get rid of the 'conoid' variable and just
use address.objectId where needed.


-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, Jun 11, 2019 at 09:32:55AM -0400, Alvaro Herrera wrote:
> Yeah, looks better.  I think the error message should be a normal elog()
> cache failure, though ... at least in the COMMENT case, the obj-does-not-
> exist message is supposed to be thrown by get_object_address(), before
> check_object_ownership is called.
>
> As a matter of style, I would get rid of the 'conoid' variable and just
> use address.objectId where needed.

OK.  I have included both your comments, and committed the patch down
to 9.5 where it applies.  Thanks for the feedback!

s/unathorized/unauthorized/ by the way.
--
Michael

On 2019-Jun-12, Michael Paquier wrote:

> On Tue, Jun 11, 2019 at 09:32:55AM -0400, Alvaro Herrera wrote:
> > Yeah, looks better.  I think the error message should be a normal elog()
> > cache failure, though ... at least in the COMMENT case, the obj-does-not-
> > exist message is supposed to be thrown by get_object_address(), before
> > check_object_ownership is called.
> > 
> > As a matter of style, I would get rid of the 'conoid' variable and just
> > use address.objectId where needed.
> 
> OK.  I have included both your comments, and committed the patch down
> to 9.5 where it applies.  Thanks for the feedback!

Thanks for fixing :-)

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> On 12 Jun 2019, at 04:36, Michael Paquier <michael@paquier.xyz> wrote:
> 
> On Tue, Jun 11, 2019 at 09:32:55AM -0400, Alvaro Herrera wrote:
>> Yeah, looks better.  I think the error message should be a normal elog()
>> cache failure, though ... at least in the COMMENT case, the obj-does-not-
>> exist message is supposed to be thrown by get_object_address(), before
>> check_object_ownership is called.
>> 
>> As a matter of style, I would get rid of the 'conoid' variable and just
>> use address.objectId where needed.
> 
> OK.  I have included both your comments, and committed the patch down
> to 9.5 where it applies.  Thanks for the feedback!

Thanks!

> s/unathorized/unauthorized/ by the way.

Ugh, sorry about that.

cheers ./daniel