Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix(gorgone): Fixes around db transaction handling #1582

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
Open

fix(gorgone): Fixes around db transaction handling #1582

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
6481690
fix(gorgone): Check if there's a db connection before checking for au…
vuntz Jul 27, 2024
d9eda97
fix(gorgone): Re-enable db autocommit on commit/rollback since this i…
vuntz Jul 27, 2024
649d969
fix(gorgone): Clear the transaction flag when connecting to the database
vuntz Jul 27, 2024
a61ce46
fix(gorgone): Cleanup db code so that callers know they don't need to…
vuntz Jul 27, 2024
e045225
fix(gorgone): Rollback db transaction when commit fails
vuntz Jul 27, 2024
d78d1d1
fix(gorgone): Make commit/rollback return if no transaction is started
vuntz Jul 27, 2024
d15b700
cleanup(gorgone): Rename transaction_begin to in_transaction for clarity
vuntz Jul 27, 2024
0716eb2
cleanup(gorgone): Avoid double-rollback when db commit fails
vuntz Jul 27, 2024
ff7cc2a
fix(gorgone): Do not accept nested db transactions, and fail when thi…
vuntz Jul 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
76 changes: 49 additions & 27 deletions gorgone/gorgone/class/db.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ sub new {

$self->{die} = defined($options{die}) ? 1 : 0;
$self->{instance} = undef;
$self->{transaction_begin} = 0;
$self->{in_transaction} = 0;
bless $self, $class;
return $self;
}
Expand Down Expand Up @@ -158,57 +158,77 @@ sub set_inactive_destroy {
}
}

sub transaction_mode {
my ($self, $mode) = @_;
sub start_transaction {
my $self = shift;

if ($self->{in_transaction}) {
$self->error('starting a transaction while already in a transaction', 'begin work');
return -1;
}

my $status;
if (!defined($self->{instance})) {
$status = $self->connect();
return -1 if ($status == -1);
}

if ($mode) {
$status = $self->{instance}->begin_work();
if (!$status) {
$self->error($self->{instance}->errstr, 'begin work');
return -1;
}
$self->{transaction_begin} = 1;
} else {
$self->{transaction_begin} = 0;
$self->{instance}->{AutoCommit} = 1;
$status = $self->{instance}->begin_work();
if (!$status) {
$self->error($self->{instance}->errstr, 'begin work');
return -1;
}
$self->{in_transaction} = 1;

return 0;
}

sub transaction_cleanup {
my $self = shift;

$self->{in_transaction} = 0;
$self->{instance}->{AutoCommit} = 1 if (defined($self->{instance}));

return 0;
}

sub commit {
my ($self) = @_;

# Commit only if autocommit isn't enabled
if ($self->{instance}->{AutoCommit} != 1) {
if (!defined($self->{instance})) {
$self->{transaction_begin} = 0;
return -1;
}
if (!$self->{in_transaction}) {
$self->error('commit outside of a transaction', 'commit');
return 0;
}

my $status = $self->{instance}->commit();
$self->{transaction_begin} = 0;
if (!defined($self->{instance})) {
$self->transaction_cleanup();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a log here or in the transaction_cleanup() sub so users know why it failed ?

Suggested change
$self->transaction_cleanup();
$self->transaction_cleanup();
$self->{logger}->writeLogError("Database connection is not defined, please check configuration and database health.");

return -1;
}

if (!$status) {
$self->error($self->{instance}->errstr, 'commit');
return -1;
}
my $status = $self->{instance}->commit();

if (!$status) {
# Note that the call to error() does a rollback, which is what we want
# in case of failure
# https://stackoverflow.com/questions/24316603/why-should-i-rollback-after-failed-commit
$self->error($self->{instance}->errstr, 'commit');
return -1;
}

$self->transaction_cleanup();

return 0;
}

sub rollback {
my ($self) = @_;

if (!$self->{in_transaction}) {
$self->error('rollback outside of a transaction', 'rollback');
return;
}

$self->{instance}->rollback() if (defined($self->{instance}));
$self->{transaction_begin} = 0;
$self->transaction_cleanup();
}

sub kill {
Expand All @@ -229,6 +249,8 @@ sub connect() {
my $self = shift;
my ($status, $count) = (0, 0);

$self->{in_transaction} = 0;

while (1) {
$self->{port} = 3306 if (!defined($self->{port}) && $self->{type} eq 'mysql');
if (defined($self->{dsn})) {
Expand Down Expand Up @@ -319,7 +341,7 @@ sub error {
Query: $query
";
$self->{logger}->writeLogError($error);
if ($self->{transaction_begin} == 1) {
if ($self->{in_transaction} == 1) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seem strange to me that error() call rollback() which can call error()
maybe you can delete one of the 2 function ?
I haven't tested but maybe this error() sub should call $self->{instance}->rollback() instead ?

$self->rollback();
}
$self->disconnect();
Expand Down
8 changes: 4 additions & 4 deletions gorgone/gorgone/class/sqlquery.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,7 +87,7 @@ sub transaction_query_multi {

my ($status, $sth);

$status = $self->transaction_mode(1);
$status = $self->begin_transaction();
return -1 if ($status == -1);

($status, $sth) = $self->{db_centreon}->query({ query => $options{request}, prepare_only => 1 });
Expand Down Expand Up @@ -119,7 +119,7 @@ sub transaction_query {
my ($self, %options) = @_;
my $status;

$status = $self->transaction_mode(1);
$status = $self->begin_transaction();
return -1 if ($status == -1);

($status) = $self->do(request => $options{request});
Expand All @@ -134,10 +134,10 @@ sub transaction_query {
return 0;
}

sub transaction_mode {
sub begin_transaction {
my ($self) = @_;

return $self->{db_centreon}->transaction_mode($_[1]);
return $self->{db_centreon}->begin_transaction();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you changed the function transaction_mode to start_transaction in db.pm, where does begin_transaction is defined ?

};

sub commit {
Expand Down
4 changes: 1 addition & 3 deletions gorgone/gorgone/modules/centreon/autodiscovery/services/discovery.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ sub new {
sub database_init_transaction {
my ($self, %options) = @_;

my $status = $self->{class_object_centreon}->{db_centreon}->transaction_mode(1);
my $status = $self->{class_object_centreon}->{db_centreon}->begin_transaction();
if ($status == -1) {
$self->{logger}->writeLogError("$@");
return -1;
Expand All @@ -89,7 +89,6 @@ sub database_commit_transaction {
return -1;
}

$self->{class_object_centreon}->transaction_mode(0);
return 0;
}

Expand All @@ -99,7 +98,6 @@ sub database_error_rollback {
$self->{logger}->writeLogError($options{message});
eval {
$self->{class_object_centreon}->rollback();
$self->{class_object_centreon}->transaction_mode(0);
};
if ($@) {
$self->{logger}->writeLogError("$@");
Expand Down
4 changes: 1 addition & 3 deletions gorgone/gorgone/modules/core/proxy/hooks.pm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -616,7 +616,7 @@ sub setlogs {
my $ctime_recent = 0;
# Transaction. We don't use last_id (problem if it's clean the sqlite table).
my $status;
$status = $options{dbh}->transaction_mode(1);
$status = $options{dbh}->begin_transaction();
return -1 if ($status == -1);

foreach (@{$options{data}->{data}->{result}}) {
Expand All @@ -639,12 +639,10 @@ sub setlogs {
if ($status == 0 && update_sync_time(dbh => $options{dbh}, id => $options{data}->{data}->{id}, ctime => $ctime_recent) == 0) {
$status = $options{dbh}->commit();
return -1 if ($status == -1);
$options{dbh}->transaction_mode(0);

$synctime_nodes->{ $options{data}->{data}->{id} }->{ctime} = $ctime_recent if ($ctime_recent != 0);
} else {
$options{dbh}->rollback();
$options{dbh}->transaction_mode(0);
return -1;
}

Expand Down