Eli Mesika has posted comments on this change. Change subject: core: Add ExternalVariable entity ......................................................................
Patch Set 2: (6 comments) http://gerrit.ovirt.org/#/c/27934/2/packaging/dbscripts/external_variable_sp.sql File packaging/dbscripts/external_variable_sp.sql: Line 2: -- Table external_variable Line 3: -- Line 4: Line 5: -- UpsertExternalVariable is used in fence_kdump listener Line 6: CREATE OR REPLACE FUNCTION UpsertExternalVariable( Why taking a different approach of all DAOs that implement insert/update separately and decide which to do in the BLL level ??? Line 7: v_var_name VARCHAR(50), Line 8: v_var_value VARCHAR(255), Line 9: v_updated TIMESTAMP WITH TIME ZONE) Line 10: RETURNS VOID Line 5: -- UpsertExternalVariable is used in fence_kdump listener Line 6: CREATE OR REPLACE FUNCTION UpsertExternalVariable( Line 7: v_var_name VARCHAR(50), Line 8: v_var_value VARCHAR(255), Line 9: v_updated TIMESTAMP WITH TIME ZONE) This should not be passed as parameter, rather it should be set from LOCALTIMESTAMP Please grep LOCALTIMESTAMP on dbscripts and see how we use that Line 10: RETURNS VOID Line 11: AS $procedure$ Line 12: DECLARE Line 13: db_updated TIMESTAMP WITH TIME ZONE; Line 13: db_updated TIMESTAMP WITH TIME ZONE; Line 14: BEGIN Line 15: -- set updated to db now if not provided Line 16: db_updated := v_updated; Line 17: IF db_updated IS NULL THEN Redundant IF block , see my 1st comment on that You can give a default value of NULL on column definition and it will be done on each insert Line 18: SELECT NOW() INTO db_updated; Line 19: END IF; Line 20: Line 21: UPDATE external_variable Line 20: Line 21: UPDATE external_variable Line 22: SET Line 23: var_value = v_var_value, Line 24: updated = db_updated Please rename updated=> _update_date like other tables of engine Line 25: WHERE var_name = v_var_name; Line 26: Line 27: IF NOT found THEN Line 28: INSERT INTO external_variable( http://gerrit.ovirt.org/#/c/27934/2/packaging/dbscripts/upgrade/03_05_0490_add_external_variable.sql File packaging/dbscripts/upgrade/03_05_0490_add_external_variable.sql: Line 2: -- Table is used to exchange data with external systems Line 3: -- Line 4: CREATE TABLE external_variable ( Line 5: var_name VARCHAR(50) NOT NULL, Line 6: var_value VARCHAR(255), I would go here for text data type as in vdc_options Line 7: updated TIMESTAMP WITH TIME ZONE NOT NULL, Line 8: CONSTRAINT pk_external_variable PRIMARY KEY(var_name) Line 9: ) WITH OIDS; Line 10: Line 7: updated TIMESTAMP WITH TIME ZONE NOT NULL, Line 8: CONSTRAINT pk_external_variable PRIMARY KEY(var_name) Line 9: ) WITH OIDS; Line 10: Line 11: CREATE INDEX idx_external_variable_updated ON external_variable(updated); I don't see anything in the SPs that selected according to the updated column, is this really necessary ? -- To view, visit http://gerrit.ovirt.org/27934 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I61ffd646c102c01c34478309d072f74a8f7d7edf Gerrit-PatchSet: 2 Gerrit-Project: ovirt-engine Gerrit-Branch: master Gerrit-Owner: Martin Peřina <[email protected]> Gerrit-Reviewer: Barak Azulay <[email protected]> Gerrit-Reviewer: Eli Mesika <[email protected]> Gerrit-Reviewer: Martin Peřina <[email protected]> Gerrit-Reviewer: Oved Ourfali <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ Engine-patches mailing list [email protected] http://lists.ovirt.org/mailman/listinfo/engine-patches
