http://www.slony.info/bugzilla/show_bug.cgi?id=137

--- Comment #9 from Steve Singer <ssinger at ca.afilias.info> 2011-11-15 13:04:19 PST ---
I have looked at your changes.

Here is my review



1. Your unit test changes should be to the javascript version of the regression
ones not the shell ones

2. It would be nice if we could be syntax backwards compatible on EXECUTE
SCRIPT.
ie 
EXECUTE SCRIPT(set id=1, FILENAME='foo', EXECUTE ONLY ON=1, EVENT NODE=1); 

should still work. The grammer accepts this, it is the code in assign_options
that need to have an INT_OR_STR option. or something.  We would also need to
make event_node optional if only 1 ONLY ON node is specifid.  



3.  In ddlchanges.sgml item 3
I am opposed to saying in the manual that DDL can be submitted directly with
the ddlCapture SQL function.  I feel that only slonik should be calling these
slony functions.  By telling people in the manual that they can do this then we
are committing to a somewhat stable API.
I think it will also encourage people to call other slony SQL functions
directly.


The counter argument is that now that ddlChange() isn't an event it actually
can be safely called by someone other than slonik and it allows applications
(ie java, php,perl,python etc...) do do there own DDL without having to make a
system() call to slonik.  I am somewhat sympathetic to this but I remind people
that in any version prior to 2.2 this was a really bad/broken idea. Even in 2.2
calling ddlChange() outside of slonik with more than one EVENT NODE can expose
you to race conditions.


In slonik_ref.sgml
I would like to see any encouragement to calling ddlCapture() directly removed
(see above for reasons)


slony1_funcs.sql

In 2.1 the ddlSCript_complete_int() function calls repair_log_triggers() and
updateRelname() to fixup the slony sl_table and log triggers after a DDL event.
 I don't see these being used in 2.2.  I *think* the log_apply trigger needs to
perform both of these actions after executing the SQL.

slony1_base.sql

In sl_log_script definition we are using bigint for log_txid and int8 for
log_actionseq.  I see sl_log_1 sl_log_2 are defined the same way.  Is there a
difference between bigint and int8?  Should we be consistnet? Should we be
changing sl_log_1 and sl_log_2 (since the 2.2 upgrade code is dropping sl_log_1
anyway)?


slonik.c
--------------

slonik_ddl_script

We don't free res1 in the happy path, just in the error case.

If the SET ID is never used should we make it an optional element in the
grammer (script_check_stmtS)




remote_worker.c
----------------------

Create a two node test case.

create set(set id=1, origin=1);
set add table(set id=1, fully qualified name='public.foo');
subscribe set(id=1,provider=1,receiver=2);
execute script(set id=1, filename='bar.sql', execute on=2);

The script gets executed on node 2 but never on node 1.  Even though I didn't
submit a 'EXECUTE ONLY ON'.  The problem is that if a node 1 is not receiving
any subscription sets from node 2 the node 1 will never see any DDL from node 2
because the provider structures in the remoteWorker don't have it listening for
these events.

-- 
Configure bugmail: http://www.slony.info/bugzilla/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are on the CC list for the bug.