Обсуждение: Re: [COMMITTERS] pgsql: pg_event_trigger_dropped_objects: Add name/args output columns

Alvaro Herrera wrote:
> pg_event_trigger_dropped_objects: Add name/args output columns
> 
> These columns can be passed to pg_get_object_address() and used to
> reconstruct the dropped objects identities in a remote server containing
> similar objects, so that the drop can be replicated.

This is causing buildfarm member dunlin to fail:

================== pgsql.build/src/test/regress/regression.diffs ===================
*** /buildfarm/root/HEAD/pgsql.build/src/test/regress/expected/privileges.out    Tue Dec 30 13:05:24 2014
--- /buildfarm/root/HEAD/pgsql.build/src/test/regress/results/privileges.out    Tue Dec 30 13:10:49 2014
***************
*** 1323,1328 ****
--- 1323,1329 ----  DROP SCHEMA testns CASCADE; NOTICE:  drop cascades to table testns.acltest1
+ ERROR:  requested object address for object type that cannot support it SELECT d.*     -- check that entries went
away  FROM pg_default_acl d LEFT JOIN pg_namespace n ON defaclnamespace = n.oid   WHERE nspname IS NULL AND
defaclnamespace!= 0;
 

No other member so far has reported a problem here.  Not sure if that's
the strangest bit, or the fact that dunlin is reporting anything in the
first place.  I can reproduce the problem quite simply by creating an
event trigger on sql_drop and then try to drop an object not supported
by getObjectIdentityParts -- in this case it's a default ACL for tables
in the schema being dropped.

My hope was that this check would help us detect new object types that
did not have a working get_object_address representation, but since
there are already cases that don't have it, it's currently bogus.  We
could add the default ACL support without too much effort, I think, but
it would take even less effort to just remove the check.

Another possible option is to mark ALTER DEFAULT PRIVILEGES as an
unsupported command in event triggers.  I'm not sure why we have it,
particularly since we don't have GRANT and REVOKE ... but then my DDL
deparsing patch is supposed to add support for GRANT and REVOKE, so I'm
not really proposing this.

Any strong opinions?

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

Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> Alvaro Herrera wrote:
>> pg_event_trigger_dropped_objects: Add name/args output columns

> This is causing buildfarm member dunlin to fail:
> ...
> No other member so far has reported a problem here.  Not sure if that's
> the strangest bit, or the fact that dunlin is reporting anything in the
> first place.  I can reproduce the problem quite simply by creating an
> event trigger on sql_drop and then try to drop an object not supported
> by getObjectIdentityParts -- in this case it's a default ACL for tables
> in the schema being dropped.

But is there any reason to think the failure on dunlin has anything to do
with default ACLs?  I think you'd better work on understanding why there
is a platform dependency here, before you consider either removing the
regression test case or adding support for object types that it shouldn't
be hitting.
        regards, tom lane

Tom Lane wrote:

> But is there any reason to think the failure on dunlin has anything to do
> with default ACLs?  I think you'd better work on understanding why there
> is a platform dependency here, before you consider either removing the
> regression test case or adding support for object types that it shouldn't
> be hitting.

Thanks for the commit.  Now dunlin is green, but treepie displayed the
failure, so we know it's not a platform dependency but probably a timing
dependency.  The failure from treepie is

*** 1323,1328 ****
--- 1323,1329 ----  DROP SCHEMA testns CASCADE; NOTICE:  drop cascades to table testns.acltest1
+ ERROR:  requested object address for unsupported object class 28: text result "for role regressuser1 in schema testns
ontypes" SELECT d.*     -- check that entries went away   FROM pg_default_acl d LEFT JOIN pg_namespace n ON
defaclnamespace= n.oid   WHERE nspname IS NULL AND defaclnamespace != 0;
 

where 28 is OCLASS_DEFACL, which is consistent with the text result.  I
have no idea why this is being invoked but it seems clear to me now that
we need to support this case.  I will work on that on Friday, and also
check whether we need to add the AMPROC/AMOP cases and USER_MAPPING.

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

I wrote:
> Alvaro Herrera <alvherre@2ndquadrant.com> writes:
>> Alvaro Herrera wrote:
>>> pg_event_trigger_dropped_objects: Add name/args output columns

>> This is causing buildfarm member dunlin to fail:
>> ...
>> No other member so far has reported a problem here.  Not sure if that's
>> the strangest bit, or the fact that dunlin is reporting anything in the
>> first place.  I can reproduce the problem quite simply by creating an
>> event trigger on sql_drop and then try to drop an object not supported
>> by getObjectIdentityParts -- in this case it's a default ACL for tables
>> in the schema being dropped.

> But is there any reason to think the failure on dunlin has anything to do
> with default ACLs?

I substituted a more detailed version of the error message, which soon
confirmed your guess that this had something to do with default ACLs:

http://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=treepie&dt=2014-12-31%2021%3A15%3A49

What seems to be happening is that the rowsecurity test installs an event
trigger, and if that happens to be active when the "DROP SCHEMA testns"
in privileges.sql executes, you get a failure because of the default
privileges attached to the schema.  The event trigger is very short-lived,
so the failure is hard to hit; nonetheless this is at least the third such
failure in the buildfarm.

I've "fixed" this by the expedient of making rowsecurity not run as part
of a parallel group.  We cannot have regression tests that trigger such
irreproducible failures.

We should not leave it there though.  The event triggers in rowsecurity
think they only fire for policy-related events, so how come we're seeing
getObjectIdentityParts invoked on a default ACL?  And if that is expected
behavior, how is it you figure it'd be OK to not have an implementation
for every possible object type?

In the long run it might be interesting to have a test module that runs
the entire core regression suite with some appropriate event trigger in
place.  We are clearly far away from being able to do that yet, though,
and in the meantime this is not how I want to find out about event-trigger
bugs.  Creating short-lifespan event triggers has to be something that
happens only in regression tests that run by themselves.
        regards, tom lane