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.

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

Error when opening repositories management page with SQL Server #36

Open
jeckyhl opened this issue Nov 23, 2012 · 8 comments
Open

Error when opening repositories management page with SQL Server #36

jeckyhl opened this issue Nov 23, 2012 · 8 comments

Comments

@jeckyhl
Copy link
Contributor

jeckyhl commented Nov 23, 2012

My configuration is MantisBT 1.2.12 with SQL Server 2008

An error occurs when opening repositories management page :
operand text is not valid for count operator

the query is

SELECT COUNT(DISTINCT filename) FROM mantis_plugin_Source_file_table AS f
JOIN mantis_plugin_Source_changeset_table AS c
ON c.id=f.change_id
WHERE c.repo_id=?

My workaround is to use VARCHAR column type instead of TEXT

ALTER TABLE mantis_plugin_Source_file_table
ALTER COLUMN filename VARCHAR(250) NOT NULL

Maybe VARCHAR(MAX) would be better ?

@dregad
Copy link
Member

dregad commented Dec 7, 2012

I would tend to agree with your proposed fix, but I would advise to use VARCHAR(255) and not 250, as this is the maximum length for a filename in most file systems [1].

However, I am not really sure why @jreese set the filename column's type to TEXT instead of VARCHAR.

John, care to comment ? Do you see any issue with changing the schema definition ?

[1] http://en.wikipedia.org/wiki/Comparison_of_file_systems#Limits

@amyreese
Copy link
Contributor

amyreese commented Dec 7, 2012

It's a TEXT field because it stores more than just the individual filename, it also stores the entire file path from the root of the repository, which can easily and quickly exceed 255 characters. I would be hesitant to suggest changing it from TEXT for those reasons, especially since the majority of users will be running mysql or postgres where that query functions correctly. I'm not familiar with SQL Server, so I have no idea of what is or isn't possible there, but for that specific query instance, it's not really "necessary" as it just provides a bit of stats for the repo list, so could simply be omitted in cases where the DB doesn't support those semantics.

@jeckyhl
Copy link
Contributor Author

jeckyhl commented Dec 7, 2012

VARCHAR(MAX) would do the trick -- you can store as many characters as you want (up to 2 Go) in VARCHAR(MAX) field

As it is stated here:

The VARCHAR(MAX) type is a replacement for TEXT. The basic difference is that a TEXT type will always store the data in a blob whereas the VARCHAR(MAX) type will attempt to store the data directly in the row unless it exceeds the 8k limitation and at that point it stores it in a blob.

So

  • I think VARCHAR(MAX) is a good solution for SQL Server
  • ...but it is very SQL-Server-specific

@amyreese
Copy link
Contributor

amyreese commented Dec 7, 2012

Then I would suggest modifying the plugin's schema() section to check the database type, and modify the schema for that column to use varchars only on sql server. I would advise against adding a new ALTER schema row though, in favor of just returning a modified CREATE row, so that schema versions aren't out of sync between various database types. Ideally, only the initial CREATE row would be part of the conditional block, which might require altering the semantics of the block, but would be better IMO than duplicating the entire schema array entirely.

@dregad
Copy link
Member

dregad commented Dec 8, 2012

Thanks for the feedback John.

It's a TEXT field because it stores more than just the individual filename, it also stores the entire file path from the root of the repository, which can easily and quickly exceed 255 characters

In that case, the limit would be the maximum path length, which is 4096 in Linux and 32767 on NTFS. But indeed as you say, the best approach is probably to keep it as TEXT and make an MSSQL-specific schema - assuming ADOdb can actually handle this VARCHAR(MAX) thing.

@jeckyhl

  • is VARCHAR(MAX) valid for all MSSQL versions, or is it specific to 2008 only ?
  • would you be able and willing to test ? I don't have access to an MSSQL platform

I would advise against adding a new ALTER schema row though, in favor of just returning a modified CREATE row, so that schema versions aren't out of sync between various database types.

That would potentially cause issues for existing installations, unless they manually execute an ALTER TABLE, no ?

What do you think about adding a schema change for MSSQL, and a no-op upgrade step for other DB's (similar to what I did in my MantisBT oracle branch ?

@amyreese
Copy link
Contributor

amyreese commented Dec 8, 2012

My question is whether that sort of null/noop schema row is supported by the plugin manager, which uses a slightly different schema definition/system than the core of Mantis.

@dregad
Copy link
Member

dregad commented Dec 8, 2012

My question is whether that sort of null/noop schema row is supported by the plugin manager

You're right, it wouldn't be supported in the current state of the code.

For Oracle branch I had to modify install.php to take care of this no-op, so to make this work for plugins the same change would have to be made in plugin_api.php / plugin_upgrade()

@jeckyhl
Copy link
Contributor Author

jeckyhl commented Dec 8, 2012

I can't help about the schema upgrade problematic.. it seems to be more tricky than one could expect !

@dregad about your two questions:

is VARCHAR(MAX) valid for all MSSQL versions, or is it specific to 2008 only

VARCHAR(MAX) has been introduced in SQL Server 2005. It is also available in SQL Server 2012. Since it is meant to be a replacement for TEXT, I will be certainly supported in futures versions of SQL Server.

would you be able and willing to test ? I don't have access to an MSSQL platform

I switched to VARCHAR(MAX) on our Mantis production site (since John said VARCHAR(255) was too short), it works fine (but I'm not sure about the mssql driver we use). I can do some more tests if needed

dregad added a commit to dregad/mantisbt that referenced this issue Sep 17, 2017
Implement the same functionality that was added to the MantisBT
installer in 061e66b.

This is a prerequisite to fix an issue with Source Integration plugin
mantisbt-plugins/source-integration#36.

Fixes #23367
dregad added a commit to mantisbt/mantisbt that referenced this issue Sep 29, 2017
Implement the same functionality that was added to the MantisBT
installer in 061e66b.

This is a prerequisite to fix an issue with Source Integration plugin
mantisbt-plugins/source-integration#36.

Fixes #23367
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants