Forum OpenACS Development: apm callbacks during uninstall?

Posted by Stan Kaufman on
There are three callbacks into which package developers can/should write appropriate dismantling code for their packages:

  1. before_unmount
  2. before_uninstantiate
  3. before_uninstall

The sql drop-scripts get sourced after all three of these.

Does anyone have a clear idea what optimally is supposed to happen when during this complex process?

There have been several threads about how nothing uninstalls correctly, but most of these were before these callbacks were added to the apm (I think). The most useful of these is this one but it deals with a package using the CR, which invokes issues different/beyond the simpler case of packages using just the acs object system. The only packages that use these callbacks at all are bug-tracker and simulation, and these are substantially more complex than the "simple" acs-object-based situation I'm starting with.

So here's what I'm doing: I filed a bug for Forums because as is, the package fails to uninstall due to referenial integrity violations in acs_objects. Jeff pointed out that package data needs to be deleted in the before_uninstantiate callback. So I tried this: in the before_uninstantiate callback, I delete from acs_objects all objects corresponding to forums_messages and forums_messages (since those are the only acs_objects that the Forums package directly creates).

I presumed that would work, but there are still referential integrity violations. Do the notifications-related need to be wiped here, too? It's hard to see why, as the acs_objects of "notification_type", "notification_interval", and "notification_delivery_method" have null context_ids. There is an acs_object with apm_package object_type that has the context_id of the package I'm uninstantiating, but if I add a call to delete this in my before_uninstantiate, I get different referential integrity violations involving site_nodes. Sheeit.

It seems that there should be some way to figure this out other than trial-and-error. I sketched out the calling chain invoked when you click on the "Delete Package" button on the /acs-admin/apm/package-delete page here. This shows (at admittedly a chunky granularity) what's going on, but it's not at all clear to me what needs to be accomplished during those callbacks.

Can someone who Gets This explain what best practices for these callbacks are so I can add 'em in to existing and future packages?

Posted by Jeff Davis on
I don't see a lot of uses for before-unmount, but before-uninstantiate should be responsible for deleting all data associated with the package instance (meaning for forums all forums, messages, attachments, notifications etc). before-uninstall should be responsible for dropping things in the db which the package creates which need to be dropped (or are easier to drop) via tcl APIs; an example might be service contract implementations.

In general looking at the error log for which constraint name is preventing the drop is the best way to track down which objects need to be deleted. I think also looking at the package dependencies can be a guide to which things need to be deleted.

btw, Great diagram.

Posted by Jade Rubick on

Great diagram and clear thinking.

What we need is for someone to volunteer to document that these callbacks should be included in packages, and how to do so. The notes tutorial is the obvious place.

I think this is a bug in notifications. It seems to me that notifications should be on delete cascade, because yes, notifications currently do have to be deleted. And if the object a notification is based on is deleted, the notification should be deleted as well.

Can you file this as a bug on notifications?

Posted by Stan Kaufman on
Jade, I'll file the notifications bug. And once I get the callbacks figured out (both during uninstall and then install), I'll also document them--presuming I in fact *can* figure this out. ;-)

I'm in the process of working through my own demo package ("Grot") that uses the CR, Workflow, Internationalization -- and intends to Do Things Right. (A warm-up lap before the more complex stuff hopefully we'll pull off in Assessment.) At whatever point I get this done, I can add it to /contrib/packages for people to look at (I've got commit to cvs now). It might be useful as an augmentation to the current tutorial.

Posted by Stan Kaufman on
Jade, looking through notifications, it appears that the foreign keys to objects about which notifications are generated (eg forums_messages) *do* "on delete cascade":

in notification_requests:

    -- The object this request pertains to
    object_id                       integer
                                    constraint notif_request_object_id_fk
                                    references acs_objects (object_id)
                                    on delete cascade,

and in notifications:

    -- the object this notification pertains to
    object_id                       integer
                                    constraint notif_object_id_fk
                                    references acs_objects(object_id)
                                    on delete cascade,

However, the primary keys for these tables (request_id, notification_id) that have a foreign key constraint to acs_objects that *don't* on cascade delete. When these objects are deleted via the pgplsql functions, then their corresponding acs_objects get correctly deleted, but otherwise the acs_objects have to be manually deleted.

So, adding "on delete cascade" to the fk constraints should fix that, eh? Thus:

alter table notifications add constraint notif_notif_id_fk
                              foreign key (object_id)
                              references acs_objects (object_id)
                              on delete cascade;

alter table notification_requests add constraint notif_request_id_fk
                              foreign key (object_id)
                              references acs_objects (object_id)
                              on delete cascade;

Anyway, presuming this is so, I'll add a patch to my bug report that does this, including an upgrade script. Have a look and if (Jade/Jeff) you think this is correct, let me know and I'll go ahead and commit to oacs-5-1.

Posted by Don Baccus on
Now that PostgreSQL has implemented ALTER TABLE ADD CONSTRAINT etc we should pick a point in time in which we chose to upgrade the toolkit to make proper use of the ON DELETE CASCADE constraint modifier.

Life would be so much easier ... unfortunately Oracle doesn't  support the full set (ON DELETE SET NULL or ON DELETE SET DEFAULT or to some value) so we can't manage referential integrity fully in the way SQL expects oen to, but ON DELETE CASCADE's a good first step IMO.

Posted by Jeff Davis on
Should we cascade the primary key constraints which reference acs_objects? The acs_object__delete() plpgsql call does things that simply cascading the delete from acs_objects would not do. Of course most of those things are further deletes which could be cascaded.

Almost none of the object type tables currently do this and I am not sure if it's a good idea or not.

Posted by Stan Kaufman on
Jeff, I didn't survey how many of the core tables cascade delete their primary keys up to their acs_objects, but one of the first packages I checked was service contracts -- which does:
create table acs_sc_contracts (
    contract_id		     integer
			     constraint acs_sc_contracts_id_fk
			     references acs_objects(object_id) 
			     on delete cascade
			     constraint acs_sc_contracts_pk
			     primary key,
create table acs_sc_operations (
    operation_id	      integer
			      constraint acs_sc_operations_opid_fk
			      references acs_objects(object_id)
			      on delete cascade
			      constraint acs_sc_operations_pk
			      primary key,
I concluded (perhaps incorrectly) that SC was a "modern" package that came closer to implementing Best Practices than some of the older packages that are more of a direct port from 3.x.

Anyway, the reason that I left these changes dangling in unapplied patches was because the practice is inconsistently applied in the toolkit right now, and you, Don, et al. are the dudes to make this call.

Posted by Jade Rubick on
Stan, I don't know enough to make an informed judgement on this. Perhaps Don or Jeff can render a verdict on this?
Posted by Christian Paolo Vásquez Chávez on
I saw your thread and I think you guys can help me with this apm callback problem.

I was trying to uninstall the news package, but when I run the drop script I got this error:

Database operation "0or1row" failed (exception NSDB, "Query was not a statement
returning rows.")

ERROR:  update or delete on "acs_objects" violates foreign key constraint
"acs_objects_context_id_fk" on "acs_objects" DETAIL:  Key (object_id)=(559) is
still referenced from table "acs_objects".

select apm_package__delete('559');

It was because there were news items in the DB, so I drop them using this funtion:

create function inline_b0 ()
returns integer as '
            v_item_cursor2 RECORD;
RAISE NOTICE ''Entro a la funcion inline_b0()'';
    FOR v_item_cursor2 IN
        select item_id
        from news_item_revisions
        PERFORM news__delete(v_item_cursor2.item_id);
' language 'plpgsql';

When I run this from PSQL it deletes all the items.
So I run the drop script and it worked very well, so I decided to make an Tcl
Callback, I call the function this way in the before-uninstall callback:

db_exec_plsql news_f {
  select inline_b0();

Then I run the "Uninstall this package from your system" option in the APM, and postgresql stalled in the funtion content_type__drop_type()

-- drop CR content_type
select content_type__drop_type(
        'news',    -- content_type
        't',      -- drop_children_p
        't'        -- drop_table_p

I don't know really why is this going on.

I also made a .tcl page that calls the inline_b0() function, and then I run the drop script and It went perfect.
Am I doing something wrong in the apm callback?



Posted by Jade Rubick on
Paulo, there have been some recent fixes to callbacks, so you might want to make sure that you have these fixes. Some things were broken with callbacks.