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(at)ladisch(dot)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(at)paquier(dot)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(at)paquier(dot)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