On 11/21/2007 6:35 PM, Jacques Caron wrote:
> Hi,
> 
> The following patch adds a new column to sl_table: tab_lock_order, 
> and modifies the order in which tables are locked during all calls 
> that require extensive locking (i.e. all those that call 
> alterTableForReplication or alterTableRestore on a set of tables): 
> they are locked in coalesce(tab_lock_order,0),tab_id order rather 
> than random order or just tab_id order.

This is a very good idea and I certainly appreciate the patch. The 
problem is that as the Postgres main project, the Slony team is rather 
reluctant to accept new functionality into existing stable branches.

I do see how this change is entirely backwards compatible. But we'd 
definitely need a broad consensus on this.


Thanks again,
Jan


> 
> This means that by default (tab_lock_order null for all tables), they 
> are locked in tab_id order (in all those calls rather than just some 
> of them), but the order can be overridden by setting tab_lock_order 
> appropriately (this allow reordering the locks, as tab_id's cannot be 
> easily changed).
> 
> Alternatives would be to set tab_lock_order's default value to be 
> tab_id (+ not null and maybe unique + remove coalesce and maybe 
> tab_id from order by), or to order by coalesce(tab_lock_order,tab_id).
> 
> Note that I'm not sure the schema upgrade is the "right way" to do 
> it, maybe add_missing_table_field() should be used for this, and 
> obviously the schema version needed to be bumped up, not sure what 
> the best practice is for that?
> 
> Comments welcome,
> 
> Jacques.
> 
> --- slony1-1.2.12.orig/src/backend/slony1_funcs.sql     Mon Oct 22 
> 17:19:50 2007
> +++ slony1-1.2.12/src/backend/slony1_funcs.sql  Thu Nov 22 00:19:29 2007
> @@ -430,7 +430,7 @@
>   returns int4
>   as '
>   begin
> -       return 12;
> +       return 13;
>   end;
>   ' language plpgsql;
>   comment on function @NAMESPACE at .slonyVersionPatchlevel () is
> @@ -1219,6 +1219,7 @@
>                          if p_backup_node = 
> @NAMESPACE at .getLocalNodeId(''_ at CLUSTERNAME@'') then
>                                  for v_row2 in select * from 
> @NAMESPACE at .sl_table
>                                                  where tab_set = v_row.set_id
> +                                               order by 
> coalesce(tab_lock_order,0),tab_id
>                                  loop
>                                          perform 
> @NAMESPACE at .alterTableRestore(v_row2.tab_id);
>                                  end loop;
> @@ -1231,6 +1232,7 @@
> 
>                                  for v_row2 in select * from 
> @NAMESPACE at .sl_table
>                                                  where tab_set = v_row.set_id
> +                                               order by 
> coalesce(tab_lock_order,0),tab_id
>                                  loop
>                                          perform 
> @NAMESPACE at .alterTableForReplication(v_row2.tab_id);
>                                  end loop;
> @@ -1380,6 +1382,7 @@
>          if p_backup_node = @NAMESPACE at .getLocalNodeId(''_ at CLUSTERNAME@'') then
>                  for v_row in select * from @NAMESPACE at .sl_table
>                                  where tab_set = p_set_id
> +                               order by coalesce(tab_lock_order,0),tab_id
>                  loop
>                          perform @NAMESPACE at .alterTableRestore(v_row.tab_id);
>                  end loop;
> @@ -1395,6 +1398,7 @@
> 
>                  for v_row in select * from @NAMESPACE at .sl_table
>                                  where tab_set = p_set_id
> +                               order by coalesce(tab_lock_order,0),tab_id
>                  loop
>                          perform 
> @NAMESPACE at .alterTableForReplication(v_row.tab_id);
>                  end loop;
> @@ -1484,7 +1488,7 @@
>          -- This is us ... time for suicide! Restore all tables to
>          -- their original status.
>          -- ----
> -       for v_tab_row in select * from @NAMESPACE at .sl_table loop
> +       for v_tab_row in select * from @NAMESPACE at .sl_table order by 
> coalesce(tab_lock_order,0),tab_id loop
>                  perform @NAMESPACE at .alterTableRestore(v_tab_row.tab_id);
>                  perform @NAMESPACE at .tableDropKey(v_tab_row.tab_id);
>          end loop;
> @@ -1984,7 +1988,7 @@
>                          where T.tab_set = p_set_id
>                                  and T.tab_reloid = PGC.oid
>                                  and PGC.relnamespace = PGN.oid
> -                       order by tab_id
> +                       order by coalesce(tab_lock_order,0),tab_id
>          loop
>                  execute ''create trigger "_ at CLUSTERNAME@_lockedset_'' ||
>                                  v_tab_row.tab_id ||
> @@ -2060,7 +2064,7 @@
>                          where T.tab_set = p_set_id
>                                  and T.tab_reloid = PGC.oid
>                                  and PGC.relnamespace = PGN.oid
> -                       order by tab_id
> +                       order by coalesce(tab_lock_order,0),tab_id
>          loop
>                  execute ''drop trigger "_ at CLUSTERNAME@_lockedset_'' ||
>                                  v_tab_row.tab_id || ''" on '' || 
> v_tab_row.tab_fqname;
> @@ -2214,7 +2218,7 @@
>          if v_local_node_id = p_old_origin or v_local_node_id = 
> p_new_origin then
>                  for v_tab_row in select tab_id from @NAMESPACE at .sl_table
>                                  where tab_set = p_set_id
> -                               order by tab_id
> +                               order by coalesce(tab_lock_order,0),tab_id
>                  loop
>                          perform 
> @NAMESPACE at .alterTableRestore(v_tab_row.tab_id);
>                  end loop;
> @@ -2362,7 +2366,7 @@
>          if v_local_node_id = p_old_origin or v_local_node_id = 
> p_new_origin then
>                  for v_tab_row in select tab_id from @NAMESPACE at .sl_table
>                                  where tab_set = p_set_id
> -                               order by tab_id
> +                               order by coalesce(tab_lock_order,0),tab_id
>                  loop
>                          perform 
> @NAMESPACE at .alterTableForReplication(v_tab_row.tab_id);
>                  end loop;
> @@ -2441,7 +2445,7 @@
>          -- ----
>          for v_tab_row in select tab_id from @NAMESPACE at .sl_table
>                          where tab_set = p_set_id
> -                       order by tab_id
> +                       order by coalesce(tab_lock_order,0),tab_id
>          loop
>                  perform @NAMESPACE at .alterTableRestore(v_tab_row.tab_id);
>                  perform @NAMESPACE at .tableDropKey(v_tab_row.tab_id);
> @@ -3711,12 +3715,12 @@
>                  -- Create a SYNC event, run the script and generate 
> the DDL_SCRIPT event
>                  -- ----
>                  perform @NAMESPACE at .createEvent(''_ at CLUSTERNAME@'', 
> ''SYNC'', NULL);
> -               perform @NAMESPACE at .alterTableRestore(tab_id) from 
> @NAMESPACE at .sl_table where tab_set in (select set_id from 
> @NAMESPACE at .sl_set where set_origin = 
> @NAMESPACE at .getLocalNodeId(''_ at CLUSTERNAME@''));
> +               perform @NAMESPACE at .alterTableRestore(tab_id) from 
> @NAMESPACE at .sl_table where tab_set in (select set_id from 
> @NAMESPACE at .sl_set where set_origin = 
> @NAMESPACE at .getLocalNodeId(''_ at CLUSTERNAME@'')) order by 
> coalesce(tab_lock_order,0),tab_id;
>          else
>                  -- ----
>                  -- If doing "only on one node" - restore ALL tables 
> irrespective of set
>                  -- ----
> -               perform @NAMESPACE at .alterTableRestore(tab_id) from 
> @NAMESPACE at .sl_table;
> +               perform @NAMESPACE at .alterTableRestore(tab_id) from 
> @NAMESPACE at .sl_table order by coalesce(tab_lock_order,0),tab_id;
>          end if;
>          return 1;
>   end;
> @@ -3743,12 +3747,12 @@
>   begin
>          perform @NAMESPACE at .updateRelname(p_set_id, p_only_on_node);
>          if p_only_on_node = -1 then
> -               perform @NAMESPACE at .alterTableForReplication(tab_id) 
> from @NAMESPACE at .sl_table where tab_set in (select set_id from 
> @NAMESPACE at .sl_set where set_origin = 
> @NAMESPACE at .getLocalNodeId(''_ at CLUSTERNAME@''));
> +               perform @NAMESPACE at .alterTableForReplication(tab_id) 
> from @NAMESPACE at .sl_table where tab_set in (select set_id from 
> @NAMESPACE at .sl_set where set_origin = 
> @NAMESPACE at .getLocalNodeId(''_ at CLUSTERNAME@'')) order by 
> coalesce(tab_lock_order,0),tab_id;
> 
>                  return  @NAMESPACE at .createEvent(''_ at CLUSTERNAME@'', 
> ''DDL_SCRIPT'',
>                          p_set_id::text, p_script::text, p_only_on_node::text);
>          else
> -               perform @NAMESPACE at .alterTableForReplication(tab_id) 
> from @NAMESPACE at .sl_table;
> +               perform @NAMESPACE at .alterTableForReplication(tab_id) 
> from @NAMESPACE at .sl_table order by coalesce(tab_lock_order,0),tab_id;
>          end if;
>          return NULL;
>   end;
> @@ -3812,7 +3816,7 @@
>          -- ----
>          -- Restore all original triggers and rules of all sets
>          -- ----
> -       for v_row in select * from @NAMESPACE at .sl_table
> +       for v_row in select * from @NAMESPACE at .sl_table order by 
> coalesce(tab_lock_order,0),tab_id
>          loop
>                  perform @NAMESPACE at .alterTableRestore(v_row.tab_id);
>          end loop;
> @@ -3843,7 +3847,7 @@
>          -- ----
>          -- Put all tables back into replicated mode
>          -- ----
> -       for v_row in select * from @NAMESPACE at .sl_table
> +       for v_row in select * from @NAMESPACE at .sl_table order by 
> coalesce(tab_lock_order,0),tab_id
>          loop
>                  perform @NAMESPACE at .alterTableForReplication(v_row.tab_id);
>          end loop;
> @@ -4363,7 +4367,7 @@
>          -- ----
>          for v_tab_row in select tab_id from @NAMESPACE at .sl_table
>                          where tab_set = p_sub_set
> -                       order by tab_id
> +                       order by coalesce(tab_lock_order,0),tab_id
>          loop
>                  perform @NAMESPACE at .alterTableRestore(v_tab_row.tab_id);
>                  perform @NAMESPACE at .tableDropKey(v_tab_row.tab_id);
> @@ -5912,6 +5921,14 @@
>                                          ) without oids'';
>                  execute ''insert into @NAMESPACE at .sl_archive_counter
>                                          (ac_num, ac_timestamp) 
> values (0, ''''epoch''''::timestamp)'';
> +       end if;
> +
> +       -- ----
> +       -- Changes for 1.2.13
> +       -- ----
> +       if p_old IN (''1.0.2'', ''1.0.5'', ''1.0.6'', ''1.1.0'', 
> ''1.1.1'', ''1.1.2'', ''1.1.3'',''1.1.5'', ''1.1.6'', ''1.1.7'', 
> ''1.1.8'', ''1.1.9'', ''1.2.0'', ''1.2.1'', ''1.2.2'', ''1.2.3'', 
> ''1.2.4'', ''1.2.5'', ''1.2.6'', ''1.2.7'', ''1.2.8'', ''1.2.9'', 
> ''1.2.10'', ''1.2.11'', ''1.2.12'') then
> +               -- Add new column sl_table.tab_lock_order
> +               execute ''alter table @NAMESPACE at .sl_table add column 
> tab_lock_order integer'';
>          end if;
> 
>          -- In any version, make sure that the xxidin() functions are 
> defined STRICT
> 
> _______________________________________________
> Slony1-general mailing list
> Slony1-general at lists.slony.info
> http://lists.slony.info/mailman/listinfo/slony1-general


-- 
#======================================================================#
# It's easier to get forgiveness for being wrong than for being right. #
# Let's break this rule - forgive me.                                  #
#================================================== JanWieck at Yahoo.com #