Rotate property is defined but never used

Description

In DBAppender.cfc, the rotate property is set (by default to true). It is not used anywhere in the file, my assumption is that it should be used to easily toggle log rotation on and off.

Activity

Show:
Brad Wood
January 15, 2020, 4:27 PM

Oh, interesting… I see the DB Appender actually has some logic for this-- I think it’s odd the word “rotation” was used since that’s normally used for file appenders, but whatever. There seems to be some serious issues with the implementation though. It’s dependant on a last rotation date stored in memory but the last rotation date always defaults to an empty string which means the if statement that requires is to be a date would never get entered! I can’t see how this logic would have ever worked in the past, nor how it would work even with your pull request. I also have concerns about the delete possibly affecting a large number of rows and locking up the table in a real world app.

And finally, I don’t think the code in the pull is correct. You’re referencing variables.rotate but properties are not stored in the variables scope. You’d want to use the getProperty() method inherited from the abstract appender to get that.

Stephen Condon
January 15, 2020, 4:35 PM

Reviewing the pull request, you’re definitely right, it should be getProperty().

I originally found this when I was chasing down DB deadlocks coming from LogBox in a multiple application ColdBox scenario with a single database shared by each app. Our plan is to disable the log rotation, and handle it at the DB level. There is currently no way to disable it short of commenting out the call to doRotationCheck() on line 153 (I was hoping to just toggle a property).

But, it’s sounding like as implemented, the log rotation should not be used.

Brad Wood
January 15, 2020, 4:36 PM

Oh wait, it’s a negative check on the isDate(). Meaning if it’s not a date, then it would skip the return and continue to the rotation. That probably works then, but the property reference still needs fixed.

Stephen Condon
January 15, 2020, 4:43 PM

Brad, I’ll fix the pull request. Thanks!

Brad Wood
January 15, 2020, 4:51 PM

Yeah, deadlocks seem pretty likely if there’s hundreds of thousands of records getting deleted. You’d start with thousands of row locks which would then try and escalate to a table lock, and at the same time you’d have inserts trying to acquire locks too. I can think of ways I could improve it for SQL Server but it wouldn’t be compat with other DBs. And if you have any sort of clustered server setup, you may have more than one server trying to do the deletes at the same time. I would personally set up a nightly job to delete the extra rows in chunks of 1000 or so until they are cleaned out.

Assignee

Stephen Condon

Reporter

Stephen Condon

Labels

None

Components

Fix versions

Priority

Trivial
Configure