Closing opened connections #116

manmorjim · 2019-12-20T17:08:18Z

This fixes #96 by closing opened connections when it is getting the size
of the table/query by executing COUNT and when it finishes the foreign
scan iteration.

This fixes #96 by closing opened connections when it is getting the size of the table/query by executing COUNT and when it finishes the foreign scan iteration.

Algunenano · 2019-12-23T10:32:22Z

2 things:

Can we review the rest of the calls to odbc_connection and make sure those connections are closed too?

  826:  odbc_connection(options, &env, &dbc);
1051:  odbc_connection(&options, &env, &dbc);
1351:  odbc_connection(&options, &env, &dbc);
1949:  odbc_connection(&options, &env, &dbc);
2006:  odbc_connection(&options, &env, &dbc);
2104:  odbc_connection(&options, &env, &dbc);

Do we need to disconnect as part of the error handling, i.e before calling ereport or elog(ERROR)?

manmorjim · 2019-12-23T10:51:46Z

Do we need to disconnect as part of the error handling, i.e before calling ereport or elog(ERROR)?

🤔 I think so. As part of the revision I'll check also if all the connections are closed when some error occurred.

Implemented the method `odbc_disconecction` which closes connections opened after doing: - odbc_table_size - odbc_query_size - odbc_tables_list - foreign scan Tested with the "employees" MySQL database: - `select * from test.departments;` - `IMPORT FOREIGN SCHEMA "employees" FROM SERVER mysql INTO "test";` - `SELECT * FROM ODBCTablesList('mysql', 10);` - `SELECT * FROM ODBCTableSize('mysql', 'dept_emp');` - `SELECT * FROM ODBCQuerySize('mysql', 'select * from salaries');`

Algunenano · 2019-12-26T10:33:44Z

odbc_fdw.c

+static void
+odbc_disconnection(SQLHENV *env, SQLHDBC *dbc)
+{
+	if (dbc && *dbc)


This is suspicious and likely what's causing crashes on Windows builds. You are setting the contents of dbc to NULL, but is SQLHDBC declared as a NULLable type in the ODBC standard (I'm guessing it comes from there)?

In any case, the fact that we are asking ourselves this question means that this approach is wrong. Either always use a pointer or ensure you only call the disconnect once so you don't need the NULL guards.

Algunenano · 2020-01-08T11:16:42Z

I think you need to check the output of both the connect and disconnect calls. It might be that we are getting errors on them.

Algunenano · 2020-01-08T17:52:21Z

odbc_fdw.c

 		SRF_RETURN_DONE(funcctx);
+		odbc_disconnection(&env, &dbc);


This will never be called since SRF_RETURN_DONE will return from the function.

Fixed. This function uses SRF and I was trying to disconnect it using wrong handlers. I stored these handlers into the user_fctx and using them to disconnect.

Implementing the right order to close a connection: 1. disconnect with the DBC handler 2. free the DBC handler 3. free the ENV handler Fix the disconnection in `odbc_tables_list`. This method is using a set-returning function (SRF) so I added to the `user_fctx` the dbc and env handlers in order to disconnect when the iteration has finished.

odbc_fdw.c

Algunenano

LGTM. I'm not sure if dying with with ERROR here is enough or it should be FATAL, but in any case it's a great improvement. Let's get this merged and we can revisit if necessary in the future.

rafatower · 2020-02-11T08:29:12Z

Jenkins tests are failing apparently due to mysql installation and should be easy to fix. Other than that, it'd be cool to merge and deploy this.

Package update fails if MySQL is not running

Start MySQL before updating it

jgoizueta · 2020-02-17T13:10:41Z

Broken tests

Tests spontaneously broke in Travis because of an update of the mysql 5.7 packages (we are installing mysql-server-core-5.7 mysql-client-5.7 mysql-server-5.7).

It the previous working tests the base image packages needn't an update:

mysql-client-5.7 is already the newest version (5.7.28-0ubuntu0.16.04.2).
mysql-server-5.7 is already the newest version (5.7.28-0ubuntu0.16.04.2).
mysql-server-core-5.7 is already the newest version (5.7.28-0ubuntu0.16.04.2).

But now they're updated

The following packages will be upgraded:
  mysql-client-5.7 mysql-server-5.7 mysql-server-core-5.7

And the update was failing:

Setting up mysql-client-5.7 (5.7.29-0ubuntu0.16.04.1) ...
Setting up mysql-server-core-5.7 (5.7.29-0ubuntu0.16.04.1) ...
Setting up mysql-server-5.7 (5.7.29-0ubuntu0.16.04.1) ...
insserv: warning: current start runlevel(s) (empty) of script `mysql' overrides LSB defaults (2 3 4 5).
insserv: warning: current stop runlevel(s) (0 1 2 3 4 5 6) of script `mysql' overrides LSB defaults (0 1 6).
mysql_upgrade: Got error: 2002: Can't connect to local MySQL server through socket '/var/run/mysqld/mysqld.sock' (2) while connecting to the MySQL server
Upgrade process encountered error and will not continue.
mysql_upgrade failed with exit status 11
dpkg: error processing package mysql-server-5.7 (--configure):
 subprocess installed post-installation script returned error exit status 1

This is fixed by efd8ba1 and a0a8a43 by installing the packages after starting the server.

But then, access to MySQL fails with

ERROR 1698 (28000): Access denied for user 'root'@'localhost'
ERROR 1698 (28000): Access denied for user 'root'@'localhost'

Which is fixed by 44b530f by changing the root authentication 😅

jgoizueta · 2020-02-17T13:14:49Z

@Algunenano could you give your blessing to this? I'll proceed to release and deploy this if you agree.

Algunenano · 2020-02-17T13:56:36Z

LGTM. I don't think we've ever updated odbc_fdw without also upgrading PG, so good luck with that.

rafatower · 2020-03-12T09:09:18Z

@jgoizueta is this deployed?

jgoizueta · 2020-03-12T09:14:26Z

This is released in v0.5.1 but not deployed, AFAIK we still have v0.4.0 in production

Closing opened connections

0f3810d

This fixes #96 by closing opened connections when it is getting the size of the table/query by executing COUNT and when it finishes the foreign scan iteration.

manmorjim requested a review from rafatower December 20, 2019 17:35

Update NEWS

9c41443

manmorjim requested review from Algunenano and removed request for rafatower December 26, 2019 10:01

Algunenano suggested changes Dec 26, 2019

View reviewed changes

Removing handlers NULL assignations

581cb4a

Closing connection after sending DONE

a1119ee

Algunenano reviewed Jan 8, 2020

View reviewed changes

manmorjim added 2 commits January 24, 2020 12:44

Checking disconnect errors

0d21f00

Algunenano self-assigned this Jan 28, 2020

Algunenano reviewed Jan 29, 2020

View reviewed changes

odbc_fdw.c Outdated Show resolved Hide resolved

Algunenano approved these changes Jan 29, 2020

View reviewed changes

Clean up connection messages

e2fb4ba

rafatower unassigned Algunenano Feb 11, 2020

jgoizueta self-assigned this Feb 17, 2020

jgoizueta added 6 commits February 17, 2020 11:00

Merge branch 'master' into fix_close_connections

59b187d

Update NEWS

56de118

Start MySQL before updating it

efd8ba1

Package update fails if MySQL is not running

Remove mysql from apt addon installation

a0a8a43

Change MySQL root authentication

44b530f

Merge pull request #120 from CartoDB/debug-travis

49cbb5b

Start MySQL before updating it

jgoizueta merged commit c4963c8 into master Feb 17, 2020

jgoizueta deleted the fix_close_connections branch February 17, 2020 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Closing opened connections #116

Closing opened connections #116

manmorjim commented Dec 20, 2019

Algunenano commented Dec 23, 2019

manmorjim commented Dec 23, 2019

Algunenano Dec 26, 2019

Algunenano commented Jan 8, 2020

Algunenano Jan 8, 2020

manmorjim Jan 24, 2020

Algunenano left a comment

rafatower commented Feb 11, 2020

jgoizueta commented Feb 17, 2020 •

edited

Loading

jgoizueta commented Feb 17, 2020

Algunenano commented Feb 17, 2020

rafatower commented Mar 12, 2020

jgoizueta commented Mar 12, 2020

Closing opened connections #116

Closing opened connections #116

Conversation

manmorjim commented Dec 20, 2019

Algunenano commented Dec 23, 2019

manmorjim commented Dec 23, 2019

Algunenano Dec 26, 2019

Choose a reason for hiding this comment

Algunenano commented Jan 8, 2020

Algunenano Jan 8, 2020

Choose a reason for hiding this comment

manmorjim Jan 24, 2020

Choose a reason for hiding this comment

Algunenano left a comment

Choose a reason for hiding this comment

rafatower commented Feb 11, 2020

jgoizueta commented Feb 17, 2020 • edited Loading

Broken tests

jgoizueta commented Feb 17, 2020

Algunenano commented Feb 17, 2020

rafatower commented Mar 12, 2020

jgoizueta commented Mar 12, 2020

jgoizueta commented Feb 17, 2020 •

edited

Loading