Date: Mon, 14 Mar 2022 00:08:07 -0700
From: Noah Misch <noah@leadboat.com>
To: "egashira.yusuke@fujitsu.com" <egashira.yusuke@fujitsu.com>
Cc: 'Kyotaro Horiguchi' <horikyota.ntt@gmail.com>,
	"pgsql-bugs@lists.postgresql.org" <pgsql-bugs@lists.postgresql.org>
Subject: Re: Reconnect a single connection used by multiple threads in
 embedded SQL in C application causes error.
Message-ID: <20220314070807.GA2317912@rfd.leadboat.com>
References: <20220223050015.GC3838703@rfd.leadboat.com>
 <TYWPR01MB72023E15214B3EF77D53D0A3FF3E9@TYWPR01MB7202.jpnprd01.prod.outlook.com>
 <TYWPR01MB72027307B41182F8A12524ADFF039@TYWPR01MB7202.jpnprd01.prod.outlook.com>
 <20220303.101202.1438042491958386230.horikyota.ntt@gmail.com>
 <TYWPR01MB7202C109CDE4D7A60D048F9CFF059@TYWPR01MB7202.jpnprd01.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: 
 <TYWPR01MB7202C109CDE4D7A60D048F9CFF059@TYWPR01MB7202.jpnprd01.prod.outlook.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
Archived-At: 
 <https://www.postgresql.org/message-id/20220314070807.GA2317912%40rfd.leadboat.com>
Precedence: bulk

On Fri, Feb 25, 2022 at 11:29:55AM +0000, egashira.yusuke@fujitsu.com wrote:
> > Would you like to propose a patch?
> 
> Sure. I will work on creating a document patch.
> 
> However, I found another problem while testing to make sure that the use cases were valid.
> 
> >      - Each thread does its own CONNECT, DISCONNECT, and other commands. 
> >        Each thread has its own default connection. No sharing at all."
> 
> In that case, if I execute CONNECT with same connection-string on each threads without connection-name, the later CONNECT was skipped.
> Then, ECPGdebug shows following message.
> 
>   ECPGconnect: connection identifier (null) is already in use
> 
> This seems to indicate that CONNECT need to specify connection-name even if we use the default connection in each thread.
> This also seems to me to be an undesirable behavior in multiple thread.

Agreed.  It might have been a nice feature for every thread to have its own
nameless connection.

> Or do we need to name all connections if we use multiple connections, even in multiple thread?

Yes.

On Fri, Mar 04, 2022 at 11:49:39AM +0000, egashira.yusuke@fujitsu.com wrote:
> > It looks like too verbose.  Couldn't it be summarized as something
> > like this?
> > 
> > > Current connection is managed in a per-thread manner. For the first
> > > use on a thread, it is the most recently used one in the process.

There's no behavior specific to "the first use on a thread", and "most
recently used" is not a factor.  Avoid those phrases.  (In a thread that has
never opened a connection, each command that retrieves the current connection
will retrieve the non-thread-specific current connection.)

> Yes.
> I thought that I needed a detailed explanation of the current connection for the note below, but this certainly seems to be summable.

For this particular paragraph, I liked your v1's level of verbosity more than
this compact version.

> > This doesn't look like a API documentation but a procedual
> > documentation.  IMO there's no particular reason that we should
> > officially concretely suggest how mulitple connectinos are managed on
> > somebody's application.  And part of the section is already in the
> > documentation.

I'm okay with your removal of that paragraph.  As a general matter of
documentation strategy, it's valid for our manual to contain both API docs and
higher-level advice about known strategies for combining the APIs into an
effective program.

> > |  <para>
> > |   If your application uses multiple threads of execution, they cannot share a
> > |   connection concurrently. You must either explicitly control access to the connection
> > |   (using mutexes) or use a connection for each thread.
> > |  </para>
> > 
> > So, it would boil down to the caution about disconnection.
> > 
> > > Note that connections are shared among all threads.  Be careful not
> > > to DISCONNECT a connection that is to be used in another thread.
> 
> I wanted to make a document that could avoid the use cases that I reported, so I tried to write a note according to the occurrence conditions.
> However, this was also too verbose, so I rewrote the note as you suggested.
> And since there is no mention of the current connection, I changed the section to add it to "36.2.3. Closing a Connection".
> 
> I think I might be better to add "In the worst case, this may cause an application crash." to this note so that users can avoid the problem more strongly.
> However, this sentence can also be read as claiming that there is an implementation problem.
> Should not I add this sentence?

It's okay to admit implementation problems.  I might write "Behavior is
undefined if the current thread is not the thread that opened the connection."
Referring to undefined behavior is a customary way for API docs to imply that
a crash is possible.  If one wanted to be even more precise, "Behavior is
undefined if the connection is the thread-specific current connection of any
other thread."  However, the more-precise wording is harder to understand.
Also, being more precise now could limit our freedom to change behavior later.

> --- a/doc/src/sgml/ecpg.sgml
> +++ b/doc/src/sgml/ecpg.sgml
> @@ -222,10 +222,15 @@ EXEC SQL CONNECT TO <replaceable>target</replaceable> <optional>AS <replaceable>
>    <para>
>     The <replaceable>connection-name</replaceable> is used to handle
>     multiple connections in one program.  It can be omitted if a
> -   program uses only one connection.  The most recently opened
> -   connection becomes the current connection, which is used by default
> -   when an SQL statement is to be executed (see later in this
> -   chapter).
> +   program uses only one connection.
> +  </para>
> +
> +  <para>
> +   The most recently opened connection becomes the current connection.

This is true.  Disconnecting also changes the current connection, to another
one of the open connections.  Please document that somewhere appropriate.

> +   The current connection is managed in a per-thread manner.  For the
> +   first use on a thread, it is the most recently opened one in the
> +   process.  The current connection is used by default when an SQL
> +   statement is to be executed (see later in this chapter).
>    </para>
>  
>    <para>
> @@ -276,8 +281,8 @@ EXEC SQL CONNECT TO :target USER :user USING :passwd;
>  
>    <para>
>     SQL statements in embedded SQL programs are by default executed on
> -   the current connection, that is, the most recently opened one.  If
> -   an application needs to manage multiple connections, then there are
> +   the current connection, that is, the most recently opened one in each thread.

Likewise, this is not accurate if a disconnect of the current connection
happened after the most recent open.  You could simply delete this claim,
which would avoid defining the current connection in two places.

> +   If an application needs to manage multiple connections, then there are
>     three ways to handle this.
>    </para>