Fixes #1164 - Peloton connects to nonexistent database by eribeiro · Pull Request #1224 · cmu-db/peloton

eribeiro · 2018-03-25T20:59:30Z

No description provided.

coveralls · 2018-03-25T22:11:13Z

Coverage increased (+0.006%) to 76.973% when pulling 47f529d on eribeiro:1164-inexistent-db into a9d83a0 on cmu-db:master.

schedutron · 2018-03-26T02:43:21Z

@eribeiro Thanks for the PR, I just forgot to inform you about doing a separate PR as I’m busy with a conference this week. However, please fix the undeclared identifier error for client. Then the code should compile fine.

ChTimTsubasa · 2018-03-26T18:06:04Z

I would review this when this PR is ready.

eribeiro · 2018-03-26T21:09:41Z

@schedutron @ChTimTsubasa My bad! I was rebasing with current master late last night and didn't see this "little" detail. Thanks for pointing out.

update: gonna fix the new test asap and then I think it will be ready to review. Thanks!

ChTimTsubasa

Should add JDBC test to see if the response is correct.

ChTimTsubasa · 2018-03-28T23:06:07Z

src/catalog/catalog.cpp

+bool Catalog::CheckDatabaseExists(const std::string &database_name) {
+	auto pg_database = DatabaseCatalog::GetInstance();
+	auto &txn_manager = concurrency::TransactionManagerFactory::GetInstance();
+	auto txn = txn_manager.BeginTransaction();


Instead of starting a txn inside Catalog, it would be better starting a new txn outside and pass it into the function. It would be more consistent with the form of other functions in Catalog.

ChTimTsubasa · 2018-03-28T23:08:50Z

test/catalog/catalog_test.cpp

+  catalog::Catalog::GetInstance()->CreateDatabase("EMP_DB", txn);
+
+  exists = catalog::Catalog::GetInstance()->CheckDatabaseExists("EMP_DB");
+  EXPECT_EQ(true, exists);


This would be false as the transaction created at line 259 has not been committed yet and therefore, the new database would not be visible to others.

Snap! This is one of the reasons the tests are failing. Thanks for pointing out.

ChTimTsubasa · 2018-03-28T23:10:47Z

src/network/postgres_protocol_handler.cpp

+      } else {
+          auto error_msg = "Database " + value + " doesn't exist";
+          auto error_code = "3D000"; // invalid_catalog_name
+          SendErrorResponse({


Have you compared the responses you sent here with the Postgres's response? You can try to capture the packet using Wireshark.

Yup, I have captured Postgres' responses using wireshark. I can capture Peloton's and compare the two.

tcm-marcel · 2018-04-03T15:17:15Z

@eribeiro Can you add the changes @ChTimTsubasa requested?

eribeiro · 2018-04-04T01:35:14Z

@tcm-marcel yes, gonna change the PR tomorrow. Thanks for the review @ChTimTsubasa!

eribeiro · 2018-04-04T20:27:40Z

@ChTimTsubasa ~~Could you please give me some pointers about adding the JDBC test? Which file should I insert this new test? There's an automated way of testing the jdbc tests via CLI?~~

I see the script test_jdbc.sh should be run with something along these lines: ./test_jdbc.sh PelotonBasicTest localhost 15721. It run, but looks like it doesn't execute the test.

ChTimTsubasa

Can you add a test case under ./test/network/ to address your fix.

You might want to follow some other test cases in the same directory such as simple_query_test.cpp, but instead your test should just boot the peloton service and have two clients connect to the server. One client should connect to an existing database while another should connect to a non-existing database. Make sure they get correct response.

eribeiro · 2018-04-05T19:36:07Z

Ok, I am gonna add those tests asap. Btw, I was able to get the a basic jdbc tests running. Some observations below when running with the following JDBC URL: jdbc:postgresql://localhost:<port>/foo

This is the stack trace on Postgres connecting to a non existent database (foo):

Exception in thread "main" org.postgresql.util.PSQLException: FATAL: database "foo" does not exist
	at org.postgresql.core.v3.ConnectionFactoryImpl.readStartupMessages(ConnectionFactoryImpl.java:730)
	at org.postgresql.core.v3.ConnectionFactoryImpl.openConnectionImpl(ConnectionFactoryImpl.java:231)
	at org.postgresql.core.ConnectionFactory.openConnection(ConnectionFactory.java:66)
	at org.postgresql.jdbc.PgConnection.<init>(PgConnection.java:211)
	at org.postgresql.Driver.makeConnection(Driver.java:407)
	at org.postgresql.Driver.connect(Driver.java:275)
	at java.sql.DriverManager.getConnection(DriverManager.java:664)
	at java.sql.DriverManager.getConnection(DriverManager.java:247)
	at PelotonBasicTest.makeConnection(PelotonBasicTest.java:149)
	at PelotonBasicTest.<init>(PelotonBasicTest.java:144)
	at PelotonBasicTest.SingleTest(PelotonBasicTest.java:807)
	at PelotonBasicTest.main(PelotonBasicTest.java:801)

This is the stack trace of Peloton after the patch connecting to same non existent database:

Exception in thread "main" org.postgresql.util.PSQLException: FATAL: Database "foo" doesn't exist
	at org.postgresql.core.v3.ConnectionFactoryImpl.doAuthentication(ConnectionFactoryImpl.java:451)
	at org.postgresql.core.v3.ConnectionFactoryImpl.openConnectionImpl(ConnectionFactoryImpl.java:223)
	at org.postgresql.core.ConnectionFactory.openConnection(ConnectionFactory.java:66)
	at org.postgresql.jdbc.PgConnection.<init>(PgConnection.java:211)
	at org.postgresql.Driver.makeConnection(Driver.java:407)
	at org.postgresql.Driver.connect(Driver.java:275)
	at java.sql.DriverManager.getConnection(DriverManager.java:664)
	at java.sql.DriverManager.getConnection(DriverManager.java:247)
	at PelotonBasicTest.makeConnection(PelotonBasicTest.java:149)
	at PelotonBasicTest.<init>(PelotonBasicTest.java:144)
	at PelotonBasicTest.SingleTest(PelotonBasicTest.java:807)
	at PelotonBasicTest.main(PelotonBasicTest.java:801)

There was a code and message for severity ('S' - "FATAL") as described here: https://www.postgresql.org/docs/9.4/static/protocol-error-fields.html that I added on the lastest commit, while the "3D000" error code came from here: https://www.postgresql.org/docs/9.4/static/errcodes-appendix.html

I see that if I don't provide the database name after the slash (jdbc:postgresql://localhost:15721/) then Peloton returns a empty value for the database name. Any idea of why this is happening?

Exception in thread "main" org.postgresql.util.PSQLException: Database "" doesn't exist
	at org.postgresql.core.v3.ConnectionFactoryImpl.doAuthentication(ConnectionFactoryImpl.java:451)
	at org.postgresql.core.v3.ConnectionFactoryImpl.openConnectionImpl(ConnectionFactoryImpl.java:223)
	at org.postgresql.core.ConnectionFactory.openConnection(ConnectionFactory.java:66)
	at org.postgresql.jdbc.PgConnection.<init>(PgConnection.java:211)
	at org.postgresql.Driver.makeConnection(Driver.java:407)
	at org.postgresql.Driver.connect(Driver.java:275)
	at java.sql.DriverManager.getConnection(DriverManager.java:664)
	at java.sql.DriverManager.getConnection(DriverManager.java:247)
	at PelotonBasicTest.makeConnection(PelotonBasicTest.java:149)
	at PelotonBasicTest.<init>(PelotonBasicTest.java:144)
	at PelotonBasicTest.SingleTest(PelotonBasicTest.java:807)
	at PelotonBasicTest.main(PelotonBasicTest.java:801)

ChTimTsubasa · 2018-04-06T14:35:39Z

The exception looks like that you are implementing the database check correctly. Can you try some test cases with "default_database", which is the default existing database?

eribeiro · 2018-04-06T17:34:21Z

@ChTimTsubasa Firstly, thanks for the patience and support. Well, I have added a quite simple test -- simple_connection_test.cpp -- in test/network directory. It tries two connections with a invalid and a valid database name. Please, let me know what I can improve on this file. There is a need for JDBC test?

But I must say that I was unable to register it as a valid test so I resorted to a quick-and-dirty hack of renaming to an already registered name and then I could test it. Could you tell me how can I register a new test file to execute via ctest?

Thanks!

ChTimTsubasa

The test case looks good. No need to worry about not having a registered test. You cannot do that because of Singleton. That is not your problem.

I think this PR is good and ready to merge.

pervazea · 2018-04-09T15:35:01Z

test/network/simple_connection_test.cpp

+  } catch (const std::exception &e) {
+    auto msg = e.what();
+    LOG_INFO("[SimpleConnectionTest] Exception occurred: %s", msg);
+    EXPECT_TRUE(true);


You are catching the exception here, but not checking if it is the expected failure type?
You should check that the failure is the expected failure.

pervazea · 2018-04-09T15:35:44Z

test/network/simple_connection_test.cpp

+    server.SetPort(port);
+    server.SetupServer();
+  } catch (peloton::ConnectionException &exception) {
+    LOG_INFO("[LaunchServer] exception when launching server");


What happens here if the server generates an exception?

pervazea · 2018-04-09T15:37:02Z

I don't understand the comment about "not being able to register a test". How did you try to add the new test?

pmenon · 2018-05-21T20:08:59Z

@eribeiro What is the status of this?

edwardoliveira · 2018-06-14T10:25:48Z

@pmenon Hi,

TL;DR: it is stalled. :(

The code works(ed), but it lacks the last (?) round of review since I've got swamped by work commitments.

I can try to resume work on it if it is already relevant, but if anyone else would like to adopt this PR feel free to do so, no problem. Afaik, it is almost ready to commit once the tests are in place and conflicts resolved. :)

mengranwo added bug in progress labels Mar 26, 2018

ChTimTsubasa self-requested a review March 26, 2018 18:05

ChTimTsubasa suggested changes Mar 28, 2018

View reviewed changes

eribeiro added 2 commits April 4, 2018 17:13

Fixes #1164 - Peloton connects to nonexistent database

9af0d70

Address review comments

3ca5b78

Fix method signature

3448962

ChTimTsubasa approved these changes Apr 5, 2018

View reviewed changes

ChTimTsubasa added accepted in progress and removed bug in progress accepted labels Apr 5, 2018

ChTimTsubasa suggested changes Apr 5, 2018

View reviewed changes

eribeiro and others added 2 commits April 5, 2018 15:47

Add quotes around database name

7239083

Add severity to error message

d6ae66a

Add simple_connection_test to test suite

9b5cf2e

ChTimTsubasa approved these changes Apr 7, 2018

View reviewed changes

ChTimTsubasa added accepted and removed in progress labels Apr 7, 2018

pervazea removed the accepted label Apr 9, 2018

pervazea self-requested a review April 9, 2018 15:30

pervazea suggested changes Apr 9, 2018

View reviewed changes

pervazea added the in progress label Apr 9, 2018

pervazea and others added 2 commits April 12, 2018 10:43

Merge branch 'master' into 1164-inexistent-db

47f529d

Merge branch 'master' into 1164-inexistent-db

a7fcc06

pmenon mentioned this pull request May 21, 2018

Peloton connects to nonexistent database #1164

Closed

Conversation

eribeiro commented Mar 25, 2018

Uh oh!

coveralls commented Mar 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

schedutron commented Mar 26, 2018

Uh oh!

ChTimTsubasa commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eribeiro commented Mar 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChTimTsubasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tcm-marcel commented Apr 3, 2018

Uh oh!

eribeiro commented Apr 4, 2018

Uh oh!

eribeiro commented Apr 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChTimTsubasa left a comment

Choose a reason for hiding this comment

Uh oh!

eribeiro commented Apr 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChTimTsubasa commented Apr 6, 2018

Uh oh!

eribeiro commented Apr 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ChTimTsubasa left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pervazea commented Apr 9, 2018

Uh oh!

pmenon commented May 21, 2018

Uh oh!

edwardoliveira commented Jun 14, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

coveralls commented Mar 25, 2018 •

edited

Loading

ChTimTsubasa commented Mar 26, 2018 •

edited

Loading

eribeiro commented Mar 26, 2018 •

edited

Loading

eribeiro commented Apr 4, 2018 •

edited

Loading

eribeiro commented Apr 5, 2018 •

edited

Loading

eribeiro commented Apr 6, 2018 •

edited

Loading