Date: Fri, 19 Feb 2021 15:43:23 -0300
From: Alvaro Herrera <alvherre@alvh.no-ip.org>
To: Zhihong Yu <zyu@yugabyte.com>
Cc: "David G. Johnston" <david.g.johnston@gmail.com>,
	Daniel Verite <daniel@manitou-mail.org>,
	Matthieu Garrigues <matthieu.garrigues@gmail.com>,
	Michael Paquier <michael@paquier.xyz>,
	Dave Cramer <davecramer@postgres.rocks>,
	PostgreSQL-development <pgsql-hackers@postgresql.org>,
	Craig Ringer <craig@2ndquadrant.com>,
	Vaishnavi Prabakaran <vaishnaviprabakaran@gmail.com>,
	Andres Freund <andres@anarazel.de>,
	Michael Paquier <michael.paquier@gmail.com>,
	Claudio Freire <klaussfreire@gmail.com>,
	Heikki Linnakangas <heikki.linnakangas@iki.fi>
Subject: Re: PATCH: Batch/pipelining support for libpq
Message-ID: <20210219184323.GA5038@alvherre.pgsql>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: 
 <CALNJ-vRrGUW=Uj0b5ZtdV_6=pV6Ge0pxBvfLrYxjvdhPPdKZmg@mail.gmail.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Precedence: bulk

On 2021-Jan-21, Zhihong Yu wrote:

> It seems '\\gset or \\aset is not ' would correspond to the check more
> closely.
> 
> +       if (my_command->argc != 1)
> +           syntax_error(source, lineno, my_command->first_line,
> my_command->argv[0],
> 
> It is possible that my_command->argc == 0 (where my_command->argv[0]
> shouldn't be accessed) ?

No -- process_backslash_command reads the word and sets it as argv[0].

> +               appendPQExpBufferStr(&conn->errorMessage,
> +                                 libpq_gettext("cannot queue commands
> during COPY\n"));
> +               return false;
> +               break;
> 
> Is the break necessary ? Similar comment for pqBatchProcessQueue().

Not necessary.  I removed them.

> +int
> +PQexitBatchMode(PGconn *conn)
> 
> Since either 0 or 1 is returned, maybe change the return type to bool.

I was kinda tempted to do that, but in reality a lot of libpq's API is
defined like that -- to return 0 (failure)/1 (success) as ints, not
bools.  For example see PQsendQuery().  I'm not inclined to do different
for these new functions.  (One curious case is PQsetvalue, which returns
int, and is documented as "returns zero for failure" and then uses
"return false").

> Also, the function operates on PGconn - should the function be static
> (pqBatchProcessQueue is) ?

I don't understand this suggestion.  How would an application call it,
if we make it static?

Thanks

-- 
Álvaro Herrera       Valdivia, Chile
"Cómo ponemos nuestros dedos en la arcilla del otro. Eso es la amistad; jugar
al alfarero y ver qué formas se pueden sacar del otro" (C. Halloway en
La Feria de las Tinieblas, R. Bradbury)