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
Attachments:
dom-constraint-comments-v1.patchtext/x-diff; charset=us-asciiDownload+49-1
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
Attachments:
dom-constraint-comments-v2.patchapplication/octet-stream; name=dom-constraint-comments-v2.patch; x-unix-mode=0644Download+72-2
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
Attachments:
dom-constraint-comments-v3.patchtext/x-diff; charset=us-asciiDownload+55-1
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