DB Dump of 20140223 - group_leaders table issue

[tarwyn]

User tables are currently defined as such

user_tables_2014-02-23-02_01.sql:

CREATE TABLE `group_leaders` (
  `gid` int(4) NOT NULL,
  `leadername` varchar(64) NOT NULL,
  `marknpc` varchar(64) NOT NULL DEFAULT '',
  `leadershipaa` tinyblob NOT NULL,
  `maintank` varchar(64) NOT NULL DEFAULT '',
  `assist` varchar(64) NOT NULL,
  `puller` varchar(64) NOT NULL DEFAULT '',
  PRIMARY KEY (`gid`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1;

It defines the column "leadershipaa" as NOT NULL but does not provide a default value for it. Likewise it asks for the column "assist" to be NOT NULLable but fails to provide a default.

This will cause an error in database.cpp @ 2048 in the function

void Database::SetGroupLeaderName(uint32 gid, const char* name) {
	char errbuf[MYSQL_ERRMSG_SIZE];
	char *query = 0;

	if (!RunQuery(query, MakeAnyLenString(&query, "Replace into group_leaders set gid=%lu, leadername='%s'",(unsigned long)gid,name), errbuf))
		printf("Unable to set group leader: %s\n",errbuf);

	safe_delete_array(query);
}

This SQL Statement will fail because it cannot create a record due to the leadershipaa/assist column requirements.

If this function fails, the group_leader record is never created and successive code that queries that table in order to function will not work correctly: groups do no longer function across zones. Additionally if grouped with a merc, you're technically in a group without a leader, and you couldn't invite anyone else into the group.

To fix, the table definition must change:

CREATE TABLE `group_leaders` (
  `gid` int(4) NOT NULL,
  `leadername` varchar(64) NOT NULL,
  `marknpc` varchar(64) NOT NULL DEFAULT '',
  `leadershipaa` tinyblob NULL,
  `maintank` varchar(64) NOT NULL DEFAULT '',
  `assist` varchar(64) NOT NULL DEFAULT '',
  `puller` varchar(64) NOT NULL DEFAULT '',
  PRIMARY KEY (`gid`)
) ENGINE=InnoDB DEFAULT CHARSET=latin1;

[cavedude]

Consequently, the query doesn't fail on Linux servers, which is why I never caught it before. This will be fixed for Windows folks in the next dump. Thanks for the heads up!

[cavedude]

This may be reverted to the way it was before and not make it into the next dump. Changing this schema causes a zone crash, at least on Linux.

[tarwyn]

It's rather curious that the query should work on Linux, because strictly from an SQL perspective, the original definition can only create records if the queries creating the record provide values for both leadershipaa and assist columns.

Do you actually get group_leader records on a linux server with the original schema?

Looking at the code in database.cpp, I don't see a reason why changing the schema as I suggested would cause a crash. It's all quite properly checked and at the most you should get an error message.

Does it crash if you revert the change to the leadershipaa column but leave the assist column the way I suggested it (default '')?

[cavedude]

Yes, the old schema works perfectly fine with Linux. The tables all come from Grand Creation's live database, some of them just have the data removed (accounts, character_, etc) I've also confirmed that my local test server which runs a completely different distro also creates group_leader entries fine using the old schema. This sort of thing isn't unusual at all, the OS can and will influence how MySQL behaves. Case sensitivity and how NULL is handled (which is what is happening here) are two examples of that. In addition, OS and the compiler version can influence how C++ behaves, which may also be a factor here as well.

Not arguing the change wasn't needed, because it most certainly was. Having a NOT NULL column and no default value obviously is bad news. The developer who wrote this code probably did it as an oversight, though.

The zone crash doesn't make any sense to me, either truthfully. I've only seen it once so far so I left the changes in (last night's DB should be fixed.) But, I've never seen this one before and it occurred right at a GetLeader() call when a player was being disbanded from the group (possibility the leader itself.) The data looks good, and old data is removed every time the server is reset - which for us is every morning. I monitor TGC for crashes anyway, so I'll just be sure to note if I see this one again.

[tarwyn]

Thanks for checking it out. I've just discovered an older version of the database at revision 2506 (the last SVN one) and I checked the group_leaders schema there and it's exactly what I posted above as "fix". So back then at least it was working like this for Linux and Windows alike:

2506 DB

CREATE TABLE `group_leaders` (
	`gid` INT(4) NOT NULL,
	`leadername` VARCHAR(64) NOT NULL,
	`marknpc` VARCHAR(64) NOT NULL DEFAULT '',
	`leadershipaa` TINYBLOB NULL,
	`maintank` VARCHAR(64) NOT NULL DEFAULT '',
	`assist` VARCHAR(64) NOT NULL DEFAULT '',
	`puller` VARCHAR(64) NOT NULL DEFAULT '',
	PRIMARY KEY (`gid`),
	UNIQUE INDEX `U_group_leaders_1` (`leadername`)
)

It's possible that MySQL handles it differently depending on OS. I remember reading about the fact that certain problems only raise WARNINGS on MySQL for Linux while raising ERRORS on MySQL for Windows - maybe this one of those instances too.

[cavedude]

What likely happened was PEQ was given test code before it was committed to the public, but the schema I was given was incorrect. But since it worked fine, it went unnoticed by me. Likely in the actual commit the error was caught and changed by the developer. Since PEQ already had table, I didn't source the SQL.

The old DB SVN was done by me manually. Whenever a change came down, I'd manually update the files to reflect it. So, it would have been using the correct schema. The current daily dumps as I said previously are created right from Grand Creation's live DB automatically every morning. So if TGC is using an out of date table, then the daily dump will reflect that.

Another thing to note is I don't personally use the daily dumps. They exist for the public use only. So if there is an issue the only way I am going to know about it is if somebody reports it like you have. Lots of times people talk about issues on the forums that I had no knowledge of, I guess they assume that I do

[tarwyn]

Thanks for the explanation. You may be right too, as I can easily imagine the new schema to work just fine with a few minor code changes.

These are the risks of working with a daily build