pgsql: postgres_fdw: reestablish new connection if cached one is detect

Hi Fujii-san,

On Tue, Oct 06, 2020 at 01:52:55AM +0000, Fujii Masao wrote:

postgres_fdw: reestablish new connection if cached one is detected as broken.

In postgres_fdw, once remote connections are established, they are cached
and re-used for subsequent queries and transactions. There can be some
cases where those cached connections are unavaiable, for example,
by the restart of remote server. In these cases, previously an error was
reported and the query accessing to remote server failed if new remote
transaction failed to start because the cached connection was broken.

This commit improves postgres_fdw so that new connection is remade
if broken connection is detected when starting new remote transaction.
This is useful to avoid unnecessary failure of queries when connection is
broken but can be reestablished.

lorikeet is telling that the test introduced by this commit is
unstable:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet&dt=2020-10-06%2008%3A28%3A36

Some details:
 BEGIN;
 SELECT 1 FROM ft1 LIMIT 1;
- ?column?
-----------
-        1
-(1 row)
-
+ERROR:  could not receive data from server: Software caused connection abort
+CONTEXT:  remote SQL command: START TRANSACTION ISOLATION LEVEL REPEATABLE READ
--
Michael

On 2020/10/07 11:13, Michael Paquier wrote:

Hi Fujii-san,

On Tue, Oct 06, 2020 at 01:52:55AM +0000, Fujii Masao wrote:

postgres_fdw: reestablish new connection if cached one is detected as broken.

In postgres_fdw, once remote connections are established, they are cached
and re-used for subsequent queries and transactions. There can be some
cases where those cached connections are unavaiable, for example,
by the restart of remote server. In these cases, previously an error was
reported and the query accessing to remote server failed if new remote
transaction failed to start because the cached connection was broken.

This commit improves postgres_fdw so that new connection is remade
if broken connection is detected when starting new remote transaction.
This is useful to avoid unnecessary failure of queries when connection is
broken but can be reestablished.

lorikeet is telling that the test introduced by this commit is
unstable:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet&dt=2020-10-06%2008%3A28%3A36

Thanks for letting me know this!

Some details:
BEGIN;
SELECT 1 FROM ft1 LIMIT 1;
- ?column?
-----------
-        1
-(1 row)
-
+ERROR:  could not receive data from server: Software caused connection abort
+CONTEXT:  remote SQL command: START TRANSACTION ISOLATION LEVEL REPEATABLE READ

This error means that new connection was successfully reestablished
after the cached connection was terminated, and then the above connection
error occurred when issuing "START TRANSACTION" command on that
new connection. There seems no suspicious relevant log messages in the
logfile. So I'm not sure why this error happened, yet.

Per the previous discusson at [1]/messages/by-id/CA+hUKGL3Son9iAeqgjPbXCpU_6hhZhw9X24uNO14mOC4bG0cCA@mail.gmail.com, lorikeet sometimes seems to cause
connection-relation failure in the regression test. So the cause of error
that we faced today also may be lorikeet itself.

[1]: /messages/by-id/CA+hUKGL3Son9iAeqgjPbXCpU_6hhZhw9X24uNO14mOC4bG0cCA@mail.gmail.com
/messages/by-id/CA+hUKGL3Son9iAeqgjPbXCpU_6hhZhw9X24uNO14mOC4bG0cCA@mail.gmail.com

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2020/10/07 12:54, Fujii Masao wrote:

On 2020/10/07 11:13, Michael Paquier wrote:

Hi Fujii-san,

On Tue, Oct 06, 2020 at 01:52:55AM +0000, Fujii Masao wrote:

postgres_fdw: reestablish new connection if cached one is detected as broken.

In postgres_fdw, once remote connections are established, they are cached
and re-used for subsequent queries and transactions. There can be some
cases where those cached connections are unavaiable, for example,
by the restart of remote server. In these cases, previously an error was
reported and the query accessing to remote server failed if new remote
transaction failed to start because the cached connection was broken.

This commit improves postgres_fdw so that new connection is remade
if broken connection is detected when starting new remote transaction.
This is useful to avoid unnecessary failure of queries when connection is
broken but can be reestablished.

lorikeet is telling that the test introduced by this commit is
unstable:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet&dt=2020-10-06%2008%3A28%3A36

Thanks for letting me know this!

Some details:
� BEGIN;
� SELECT 1 FROM ft1 LIMIT 1;
- ?column?
-----------
-������� 1
-(1 row)
-
+ERROR:� could not receive data from server: Software caused connection abort
+CONTEXT:� remote SQL command: START TRANSACTION ISOLATION LEVEL REPEATABLE READ

This error means that new connection was successfully reestablished
after the cached connection was terminated, and then the above connection
error occurred when issuing "START TRANSACTION" command on that
new connection. There seems no suspicious relevant log messages in the
logfile. So I'm not sure why this error happened, yet.

Per the previous discusson at [1], lorikeet sometimes seems to cause
connection-relation failure in the regression test. So the cause of error
that we faced today also may be lorikeet itself.

Since it's not good to keep the buildfarm member red, I will revert
the commit unless I come up with something even after further
investigation.

My current just guess is that PQstatus(conn) doesn't indicate
CONNECTION_BAD when the above error occurs, and which
prevents new connection from being reestablished because of
the following check.

+		if (PQstatus(entry->conn) != CONNECTION_BAD ||
+			entry->xact_depth > 0 ||
+			retry_conn)
+			PG_RE_THROW();

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2020/10/07 22:25, Fujii Masao wrote:

On 2020/10/07 12:54, Fujii Masao wrote:

On 2020/10/07 11:13, Michael Paquier wrote:

Hi Fujii-san,

On Tue, Oct 06, 2020 at 01:52:55AM +0000, Fujii Masao wrote:

postgres_fdw: reestablish new connection if cached one is detected as broken.

In postgres_fdw, once remote connections are established, they are cached
and re-used for subsequent queries and transactions. There can be some
cases where those cached connections are unavaiable, for example,
by the restart of remote server. In these cases, previously an error was
reported and the query accessing to remote server failed if new remote
transaction failed to start because the cached connection was broken.

This commit improves postgres_fdw so that new connection is remade
if broken connection is detected when starting new remote transaction.
This is useful to avoid unnecessary failure of queries when connection is
broken but can be reestablished.

lorikeet is telling that the test introduced by this commit is
unstable:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet&dt=2020-10-06%2008%3A28%3A36

Thanks for letting me know this!

Some details:
� BEGIN;
� SELECT 1 FROM ft1 LIMIT 1;
- ?column?
-----------
-������� 1
-(1 row)
-
+ERROR:� could not receive data from server: Software caused connection abort
+CONTEXT:� remote SQL command: START TRANSACTION ISOLATION LEVEL REPEATABLE READ

This error means that new connection was successfully reestablished
after the cached connection was terminated, and then the above connection
error occurred when issuing "START TRANSACTION" command on that
new connection. There seems no suspicious relevant log messages in the
logfile. So I'm not sure why this error happened, yet.

Per the previous discusson at [1], lorikeet sometimes seems to cause
connection-relation failure in the regression test. So the cause of error
that we faced today also may be lorikeet itself.

Since it's not good to keep the buildfarm member red, I will revert
the commit unless I come up with something even after further
investigation.

My current just guess is that PQstatus(conn) doesn't indicate
CONNECTION_BAD when the above error occurs, and which
prevents new connection from being reestablished because of
the following check.

+������� if (PQstatus(entry->conn) != CONNECTION_BAD ||
+����������� entry->xact_depth > 0 ||
+����������� retry_conn)
+����������� PG_RE_THROW();

The error message in discussion is reported when recv() fails and
errno=ECONNABORTED. As far as I read the code, pqReadData() marks
the connection as CONNECTION_BAD when errno=ECONNRESET,
but not when errno=ECONNABORTED. So since PQstatus(entry->conn)
doesn't indicate CONNECTION_BAD in ECONNABORTED case,
the above check is passed through, an error is re-thrown and
new connection is not reestablished.

Therefore, the easy fix is to make libpq mark the connection as
CONNECTION_BAD even in ECONNABORTED, like we do in ECONNRESET.
But is it safe to do that? Thought?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2020/10/08 0:48, Fujii Masao wrote:

On 2020/10/07 22:25, Fujii Masao wrote:

On 2020/10/07 12:54, Fujii Masao wrote:

On 2020/10/07 11:13, Michael Paquier wrote:

Hi Fujii-san,

On Tue, Oct 06, 2020 at 01:52:55AM +0000, Fujii Masao wrote:

postgres_fdw: reestablish new connection if cached one is detected as broken.

In postgres_fdw, once remote connections are established, they are cached
and re-used for subsequent queries and transactions. There can be some
cases where those cached connections are unavaiable, for example,
by the restart of remote server. In these cases, previously an error was
reported and the query accessing to remote server failed if new remote
transaction failed to start because the cached connection was broken.

This commit improves postgres_fdw so that new connection is remade
if broken connection is detected when starting new remote transaction.
This is useful to avoid unnecessary failure of queries when connection is
broken but can be reestablished.

lorikeet is telling that the test introduced by this commit is
unstable:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=lorikeet&dt=2020-10-06%2008%3A28%3A36

Thanks for letting me know this!

Some details:
� BEGIN;
� SELECT 1 FROM ft1 LIMIT 1;
- ?column?
-----------
-������� 1
-(1 row)
-
+ERROR:� could not receive data from server: Software caused connection abort
+CONTEXT:� remote SQL command: START TRANSACTION ISOLATION LEVEL REPEATABLE READ

This error means that new connection was successfully reestablished
after the cached connection was terminated, and then the above connection
error occurred when issuing "START TRANSACTION" command on that
new connection. There seems no suspicious relevant log messages in the
logfile. So I'm not sure why this error happened, yet.

Per the previous discusson at [1], lorikeet sometimes seems to cause
connection-relation failure in the regression test. So the cause of error
that we faced today also may be lorikeet itself.

Since it's not good to keep the buildfarm member red, I will revert
the commit unless I come up with something even after further
investigation.

My current just guess is that PQstatus(conn) doesn't indicate
CONNECTION_BAD when the above error occurs, and which
prevents new connection from being reestablished because of
the following check.

+������� if (PQstatus(entry->conn) != CONNECTION_BAD ||
+����������� entry->xact_depth > 0 ||
+����������� retry_conn)
+����������� PG_RE_THROW();

The error message in discussion is reported when recv() fails and
errno=ECONNABORTED. As far as I read the code, pqReadData() marks
the connection as CONNECTION_BAD when errno=ECONNRESET,
but not when errno=ECONNABORTED. So since PQstatus(entry->conn)
doesn't indicate CONNECTION_BAD in ECONNABORTED case,
the above check is passed through, an error is re-thrown and
new connection is not reestablished.

Therefore, the easy fix is to make libpq mark the connection as
CONNECTION_BAD even in ECONNABORTED, like we do in ECONNRESET.

Patch attached. This patch also changes errcode_for_socket_access()
so that it uses ERRCODE_CONNECTION_FAILURE rather than
ERRCODE_INTERNAL_ERROR as sqlerrorcode in ECONNABORTED case
like ECONNRESET. Is this sane?

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

Therefore, the easy fix is to make libpq mark the connection as
CONNECTION_BAD even in ECONNABORTED, like we do in ECONNRESET.

Patch attached. This patch also changes errcode_for_socket_access()
so that it uses ERRCODE_CONNECTION_FAILURE rather than
ERRCODE_INTERNAL_ERROR as sqlerrorcode in ECONNABORTED case
like ECONNRESET. Is this sane?

Some digging around on the net suggests that Windows will only return
WSAECONNRESET for the case where it gets a TCP RST packet from the remote
host. Other cases of TCP connection loss (e.g. timeout) are reported
as WSAECONNABORTED.

I think that our code generally only expects ECONNRESET to be reported
for a lost connection. Based on this info, we clearly need to handle
ECONNABORTED as being equivalent to that. I don't see any reason why
we'd care to distinguish the two cases.

So it seems like (a) we'd better make sure that ECONNABORTED is
recognized anyplace that we're testing for ECONNRESET, and (b) that
seems like a back-patchable bug fix, independently of the immediate
issue.

On the other hand, it seems a bit odd that we've not recognized
this issue long since. Maybe the preference for ECONNABORTED only
exists on a few Windows versions?

I've not checked your patch in detail.

regards, tom lane

I wrote:

I've not checked your patch in detail.

Bread crumb for the archives: I started a -hackers thread about this:

/messages/by-id/2621622.1602184554@sss.pgh.pa.us

regards, tom lane

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

Therefore, the easy fix is to make libpq mark the connection as
CONNECTION_BAD even in ECONNABORTED, like we do in ECONNRESET.

So in the wake of commit fe27009cb, this is what lorikeet is doing:

@@ -9028,9 +9028,7 @@
 CALL terminate_backend_and_wait('fdw_retry_check');
 SAVEPOINT s;
 SELECT 1 FROM ft1 LIMIT 1;    -- should fail
-ERROR:  server closed the connection unexpectedly
-	This probably means the server terminated abnormally
-	before or while processing the request.
+ERROR:  could not receive data from server: Software caused connection abort
 CONTEXT:  remote SQL command: SAVEPOINT s2
 COMMIT;
 -- Clean up

which is better --- the connection recovery is working --- but
obviously still not a "pass".

The only way to make this test pass as-is is to hack libpq so that
it deems ECONNABORTED to be a server crash, which frankly I do not
think is a good change. I don't see any principled reason to think
that it means that rather than a network connectivity failure.

What I've done instead as a stopgap is to adjust the test case to be
insensitive to the exact error message text. Maybe there's a better
way, but I can't think of anything besides having two (or more?)
expected-output files. That would be quite unpalatable as things
stand, though perhaps we could make it tolerable by splitting this
test off into a second test script.

Meanwhile, now that I've looked at commit 32a9c0bdf, I'm not very
happy with it:

* The control flow seems rather forced. I think it was designed
on the assumption that reindenting the existing code is forbidden.
Why not use an actual loop, instead of a goto? I also think that
it's far from obvious that the loop isn't an infinite loop; it
took me quite a while to glom onto the fact that the test inside the
PG_CATCH avoids that by checking retry_conn. Maybe a comment would
be enough to improve that, but I feel the control logic could stand
a rethink.

* The CATCH case is completely failing to clean up after itself.
At the minimum, there has to be a FlushErrorState() there.
I don't think it'd be terribly hard to drive this code to an
error-stack-overflow PANIC.

* As is so often true of proposed patches in which PG_CATCH is
thought to be an adequate way to catch an error, this is just
unbelievably fragile. It will break, and badly so, if it catches
an error that is anything other than what it is expecting ...
and it's not even particularly trying to verify that the error is
what it's expecting. It might be okay, or at least a little bit
harder to break, if it verified that the error's SQLSTATE is
ERRCODE_CONNECTION_FAILURE.

regards, tom lane

On 2020/10/11 9:16, Tom Lane wrote:

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

Therefore, the easy fix is to make libpq mark the connection as
CONNECTION_BAD even in ECONNABORTED, like we do in ECONNRESET.

So in the wake of commit fe27009cb,

Many thanks for the commit fe27009cb !!

this is what lorikeet is doing:

@@ -9028,9 +9028,7 @@
CALL terminate_backend_and_wait('fdw_retry_check');
SAVEPOINT s;
SELECT 1 FROM ft1 LIMIT 1;    -- should fail
-ERROR:  server closed the connection unexpectedly
-	This probably means the server terminated abnormally
-	before or while processing the request.
+ERROR:  could not receive data from server: Software caused connection abort
CONTEXT:  remote SQL command: SAVEPOINT s2
COMMIT;
-- Clean up

which is better --- the connection recovery is working --- but
obviously still not a "pass".

The only way to make this test pass as-is is to hack libpq so that
it deems ECONNABORTED to be a server crash, which frankly I do not
think is a good change. I don't see any principled reason to think
that it means that rather than a network connectivity failure.

What I've done instead as a stopgap is to adjust the test case to be
insensitive to the exact error message text.

For now I've not come up with better idea than current this fix.

Maybe there's a better
way, but I can't think of anything besides having two (or more?)
expected-output files. That would be quite unpalatable as things
stand, though perhaps we could make it tolerable by splitting this
test off into a second test script.

Meanwhile, now that I've looked at commit 32a9c0bdf, I'm not very
happy with it:

* The control flow seems rather forced. I think it was designed
on the assumption that reindenting the existing code is forbidden.
Why not use an actual loop, instead of a goto? I also think that
it's far from obvious that the loop isn't an infinite loop; it
took me quite a while to glom onto the fact that the test inside the
PG_CATCH avoids that by checking retry_conn. Maybe a comment would
be enough to improve that, but I feel the control logic could stand
a rethink.

Isn't it simpler and easier-to-read to just reestablish new connection again
in the retry case instead of using a loop because we retry only once?
Patch attached.

* The CATCH case is completely failing to clean up after itself.
At the minimum, there has to be a FlushErrorState() there.
I don't think it'd be terribly hard to drive this code to an
error-stack-overflow PANIC.

You're right. Sorry I forgot to take into consideration this :(
I fixed this issue in the attached patch.

* As is so often true of proposed patches in which PG_CATCH is
thought to be an adequate way to catch an error, this is just
unbelievably fragile. It will break, and badly so, if it catches
an error that is anything other than what it is expecting ...
and it's not even particularly trying to verify that the error is
what it's expecting. It might be okay, or at least a little bit
harder to break, if it verified that the error's SQLSTATE is
ERRCODE_CONNECTION_FAILURE.

"PQstatus()==CONNECTION_BAD" was checked for that purpose. But that's not enough?
Anyway, in the patch, I changed the code so that sqlstate==ERRCODE_CONNECTION_FAILURE
is checked.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

On 2020/10/11 9:16, Tom Lane wrote:

Meanwhile, now that I've looked at commit 32a9c0bdf, I'm not very
happy with it:

* The control flow seems rather forced. I think it was designed
on the assumption that reindenting the existing code is forbidden.

Isn't it simpler and easier-to-read to just reestablish new connection again
in the retry case instead of using a loop because we retry only once?
Patch attached.

Yeah, that seems better.

* As is so often true of proposed patches in which PG_CATCH is
thought to be an adequate way to catch an error, this is just
unbelievably fragile. It will break, and badly so, if it catches
an error that is anything other than what it is expecting ...
and it's not even particularly trying to verify that the error is
what it's expecting. It might be okay, or at least a little bit
harder to break, if it verified that the error's SQLSTATE is
ERRCODE_CONNECTION_FAILURE.

"PQstatus()==CONNECTION_BAD" was checked for that purpose. But that's not enough?
Anyway, in the patch, I changed the code so that sqlstate==ERRCODE_CONNECTION_FAILURE
is checked.

The reason I'm concerned about this is that we could potentially throw an
elog(ERROR) somewhere between return from libpq and the expected ereport
call in pgfdw_report_error. It wouldn't be wise to ignore such a
condition (it might be out-of-memory or some such).

Personally I'd code the check with explicit tests for *both*
PQstatus()==CONNECTION_BAD and sqlstate==ERRCODE_CONNECTION_FAILURE,
rather than just asserting that the latter must imply the former.
By my reading, pgfdw_report_error will report ERRCODE_CONNECTION_FAILURE
for any libpq-originated error condition, but not all of those will
set CONNECTION_BAD.

Other than that, this seems reasonable by eyeball (I didn't do any
testing).

regards, tom lane

On 2020/10/14 3:34, Tom Lane wrote:

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

On 2020/10/11 9:16, Tom Lane wrote:

Meanwhile, now that I've looked at commit 32a9c0bdf, I'm not very
happy with it:

* The control flow seems rather forced. I think it was designed
on the assumption that reindenting the existing code is forbidden.

Isn't it simpler and easier-to-read to just reestablish new connection again
in the retry case instead of using a loop because we retry only once?
Patch attached.

Yeah, that seems better.

* As is so often true of proposed patches in which PG_CATCH is
thought to be an adequate way to catch an error, this is just
unbelievably fragile. It will break, and badly so, if it catches
an error that is anything other than what it is expecting ...
and it's not even particularly trying to verify that the error is
what it's expecting. It might be okay, or at least a little bit
harder to break, if it verified that the error's SQLSTATE is
ERRCODE_CONNECTION_FAILURE.

"PQstatus()==CONNECTION_BAD" was checked for that purpose. But that's not enough?
Anyway, in the patch, I changed the code so that sqlstate==ERRCODE_CONNECTION_FAILURE
is checked.

The reason I'm concerned about this is that we could potentially throw an
elog(ERROR) somewhere between return from libpq and the expected ereport
call in pgfdw_report_error. It wouldn't be wise to ignore such a
condition (it might be out-of-memory or some such).

Understood.

Personally I'd code the check with explicit tests for *both*
PQstatus()==CONNECTION_BAD and sqlstate==ERRCODE_CONNECTION_FAILURE,
rather than just asserting that the latter must imply the former.
By my reading, pgfdw_report_error will report ERRCODE_CONNECTION_FAILURE
for any libpq-originated error condition, but not all of those will
set CONNECTION_BAD.

Yes, this makes sense. I updated the patch so that both sqlstate and
PQstatus() are checked. Patch attached.

Other than that, this seems reasonable by eyeball (I didn't do any
testing).

Thanks! Barring any objection, I will commit it.

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION

On 2020/10/15 16:21, Fujii Masao wrote:

On 2020/10/14 3:34, Tom Lane wrote:

Fujii Masao <masao.fujii@oss.nttdata.com> writes:

On 2020/10/11 9:16, Tom Lane wrote:

Meanwhile, now that I've looked at commit 32a9c0bdf, I'm not very
happy with it:

* The control flow seems rather forced.  I think it was designed
on the assumption that reindenting the existing code is forbidden.

Isn't it simpler and easier-to-read to just reestablish new connection again
in the retry case instead of using a loop because we retry only once?
Patch attached.

Yeah, that seems better.

* As is so often true of proposed patches in which PG_CATCH is
thought to be an adequate way to catch an error, this is just
unbelievably fragile.  It will break, and badly so, if it catches
an error that is anything other than what it is expecting ...
and it's not even particularly trying to verify that the error is
what it's expecting.  It might be okay, or at least a little bit
harder to break, if it verified that the error's SQLSTATE is
ERRCODE_CONNECTION_FAILURE.

"PQstatus()==CONNECTION_BAD" was checked for that purpose. But that's not enough?
Anyway, in the patch, I changed the code so that sqlstate==ERRCODE_CONNECTION_FAILURE
is checked.

The reason I'm concerned about this is that we could potentially throw an
elog(ERROR) somewhere between return from libpq and the expected ereport
call in pgfdw_report_error.  It wouldn't be wise to ignore such a
condition (it might be out-of-memory or some such).

Understood.

Personally I'd code the check with explicit tests for *both*
PQstatus()==CONNECTION_BAD and sqlstate==ERRCODE_CONNECTION_FAILURE,
rather than just asserting that the latter must imply the former.
By my reading, pgfdw_report_error will report ERRCODE_CONNECTION_FAILURE
for any libpq-originated error condition, but not all of those will
set CONNECTION_BAD.

Yes, this makes sense. I updated the patch so that both sqlstate and
PQstatus() are checked. Patch attached.

Other than that, this seems reasonable by eyeball (I didn't do any
testing).

Thanks! Barring any objection, I will commit it.

Pushed. Thanks!

Regards,

--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION