On 2020/09/17 15:44, Bharath Rupireddy wrote:
Thanks for the review comments. I will post a new patch soon
addressing all the comments.
Thanks a lot!
On Tue, Sep 15, 2020 at 2:49 PM Fujii Masao <masao.fu...@oss.nttdata.com> wrote:
+ PQreset(entry->conn);
Isn't using PQreset() to reconnect to the foreign server unsafe?
When new connection is established, some SET commands seem
to need to be issued like configure_remote_session() does.
But PQreset() doesn't do that at all.
This is a good catch. Thanks, I missed this point. Indeed we need to
set the session params. We can do this in two ways: 1. use
configure_remote_session() after PQreset(). 2. use connect_pg_server()
instead of PQreset() and configure_remote_session(). One problem I see
with the 2nd way is that we will be doing the checks that are being
performed in connect_pg_server() twice, which we would have done for
the first time before retrying. The required parameters such as
keywords, values, are still in entry->conn structure from the first
attempt, which can safely be used by PQreset(). So, I will go with the
1st way. Thoughts?
In 1st way, you may also need to call ReleaseExternalFD() when new connection
fails
to be made, as connect_pg_server() does. Also we need to check that
non-superuser has used password to make new connection,
as connect_pg_server() does? I'm concerned about the case where
pg_hba.conf is changed accidentally so that no password is necessary
at the remote server and the existing connection is terminated. In this case,
if we connect to the local server as non-superuser, connection to
the remote server should fail because the remote server doesn't
require password. But with your patch, we can successfully reconnect
to the remote server.
Therefore I like 2nd option. Also maybe disconnect_ps_server() needs to
be called before that. I'm not sure how much useful 1st option is.
Originally when GetConnection() establishes new connection,
it resets all transient state fields, to be sure all are clean (as the
comment says). With the patch, even when reconnecting
the remote server, shouldn't we do the same, for safe?
I guess there is no need for us to reset all the transient state
before we do begin_remote_xact() in the 2nd attempt. We retry the
connection only when entry->xact_depth <= 0 i.e. beginning of the
remote txn and the begin_remote_xact() doesn't modify any transient
state if entry->xact_depth <= 0, except for entry->changing_xact_state
= true; all other transient state is intact in entry structure. In the
error case, we will not reach the code after do_sql_command in
begin_remote_xact(). If needed, we can only set
entry->changing_xact_state to false which is set to true before
do_sql_command().
What if 2nd attempt happens with have_prep_stmt=true? I'm not sure
if this case really happens, though. But if that can, it's strange to start
new connection with have_prep_stmt=true even when the caller of
GetConnection() doesn't intend to create any prepared statements.
I think it's safer to do 2nd attempt in the same way as 1st one. Maybe
we can simplify the code by making them into common code block
or function.
entry->changing_xact_state = true;
do_sql_command(entry->conn, sql);
entry->xact_depth = 1;
entry->changing_xact_state = false;
+ PGresult *res = NULL;
+ res = PQgetResult(entry->conn);
+ PQclear(res);
Are these really necessary? I was just thinking that's not because
pgfdw_get_result() and pgfdw_report_error() seem to do that
already in do_sql_command().
If an error occurs in the first attempt, we return from
pgfdw_get_result()'s if (!PQconsumeInput(conn)) to the catch block we
added and pgfdw_report_error() will never get called. And the below
part of the code is reached only in scenarios as mentioned in the
comments. Removing this might have problems if we receive errors other
than CONNECTION_BAD or for subtxns. We could clear if any result and
just rethrow the error upstream. I think no problem having this code
here.
But the orignal code works without this?
Or you mean that the original code has the bug?
else
{
/*
* We are here, due to either some error other than CONNECTION_BAD
* occurred or connection may have broken during start of a
* subtransacation. Just, clear off any result, try rethrowing the
* error, so that it will be caught appropriately.
*/
PGresult *res = NULL;
res = PQgetResult(entry->conn);
PQclear(res);
PG_RE_THROW();
}
+ /* Start a new transaction or subtransaction if needed. */
+ begin_remote_xact(entry);
Even when there is no cached connection and new connection is made,
then if begin_remote_xact() reports an error, another new connection
tries to be made again. This behavior looks a bit strange.
When there is no cached connection, we try to acquire one, if we
can't, then no error will be thrown to the user, just we retry one
more time. If we get in the 2nd attempt, fine, if not, we will throw
the error to the user. Assume in the 1st attempt the remote server is
unreachable, we may hope to connect in the 2nd attempt. I think there
is no problem here.
With Regards,
Bharath Rupireddy.
EnterpriseDB: http://www.enterprisedb.com
--
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION
