Skip to content

Conversation

@ucko
Copy link
Contributor

@ucko ucko commented May 30, 2024

Continue to run against it when explicitly specifying a catalog (in ODBC parlance), rather than attempting to create(!) (if absent) and use freetds_test.

Split from #555.

@freddy77
Copy link
Contributor

freddy77 commented Jun 1, 2024

I suppose the reason for this change is to avoid the need to create a database with the required permissions associated. Or is there any other reasons? That should be stated in the commit message. Also worth saying that using default database is not limiting the effectiveness of the test (or at least it seems so looking at the code).

@ucko ucko force-pushed the ncbi-2024-05-stats-sandboxed branch from 07f3419 to 6996eec Compare June 13, 2024 18:21
@ucko
Copy link
Contributor Author

ucko commented Jun 13, 2024

Done:

The precise database name has no bearing on this test's effectiveness.
However, the test cases that explicitly specified a catalog (in ODBC
parlance) to TestTable historically insisted on using freetds_test,
and this test's only provision for lacking such a database was
attempting to create it itself despite not necessarily having
sufficient privileges.

On a somewhat related note (which of course deserves its own PR), it would be helpful if tests used temporary database objects to the extent possible; using nominally permanent ones is mostly no problem but can result in race conditions when running multiple tests at the same time.

@freddy77
Copy link
Contributor

Unfortunately there's a reason why using a different database. Queries we must send internally are different if the database is different from the current. To fix the permission issue I suppose we can assume the presence of both master and tempdb and use "the other" (based on current database) instead of freetds_test. About temporary tables if I remember correctly there's a reason too, related to the way database internally store temporary table names.

The precise database name has no bearing on this test's effectiveness.
However, the test cases that explicitly specified a catalog (in ODBC
parlance) to TestTable historically insisted on using freetds_test,
and this test's only provision for lacking such a database was
attempting to create it itself despite not necessarily having
sufficient privileges.

Bypass potential optimizations by formally changing to master before
explicitly supplying the specified database name, with the help of a
set_dbname copied from connect2.c.

Signed-off-by: Aaron M. Ucko <ucko@ncbi.nlm.nih.gov>
@ucko ucko force-pushed the ncbi-2024-05-stats-sandboxed branch from 6996eec to f4d23fa Compare June 13, 2024 20:00
@ucko
Copy link
Contributor Author

ucko commented Jun 13, 2024

Good point, though master may be too restricted; I've tweaked my commit to use it only as the nominal default when explicitly testing against the specified database:

Bypass potential optimizations by formally changing to master before
explicitly supplying the specified database name, with the help of a
set_dbname copied from connect2.c.

@freddy77
Copy link
Contributor

Wonderful! Testing, did some changes (appveyor, userguide).

@freddy77
Copy link
Contributor

Merged, see 37e6856

@freddy77 freddy77 closed this Jun 17, 2024
@ucko
Copy link
Contributor Author

ucko commented Jun 18, 2024

No problem. Thanks for pointing me in the right general direction and adding final touches!

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

Successfully merging this pull request may close these issues.

2 participants