Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error

On Mon, 19 Jul 2021 10:51:36 +0900
Yugo NAGATA <nagata@sraoss.co.jp> wrote:

> Hello Fabien,
> 
> On Sat, 17 Jul 2021 07:03:01 +0200 (CEST)
> Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> 
> > 
> > Hello Yugo-san,
> > 
> > > [...] One way to avoid these errors is to send Parse messages before 
> > > pipeline mode starts. I attached a patch to fix to prepare commands at 
> > > starting of a script instead of at the first execution of the command.
> > 
> > > What do you think?
> > 
> > ISTM that moving prepare out of command execution is a good idea, so I'm 
> > in favor of this approach: the code is simpler and cleaner.
> > 
> > ISTM that a minor impact is that the preparation is not counted in the 
> > command performance statistics. I do not think that it is a problem, even 
> > if it would change detailed results under -C -r -M prepared.
> 
> I agree with you. Currently, whether prepares are sent in pipeline mode or
> not depends on whether the first SQL command is placed between \startpipeline
> and \endpipeline regardless whether other commands are executed in pipeline
> or not. ISTM, this behavior would be not intuitive for users.  Therefore, 
> I think preparing all statements not using pipeline mode is not problem for now.
> 
> If some users would like to send prepares in  pipeline, I think it would be
> better to provide more simple and direct way. For example, we prepare statements
> in pipeline if the user use an option, or if the script has at least one
> \startpipeline in their pipeline. Maybe,  --pipeline option is useful for users
> who want to use pipeline mode for all queries in scirpts including built-in ones.
> However, these features seems to be out of the patch proposed in this thread.
> 
> > Patch applies & compiles cleanly, global & local make check ok. However 
> > the issue is not tested. I think that the patch should add a tap test case 
> > for the problem being addressed.
> 
> Ok. I'll add a tap test to confirm the error I found is avoidable.
> 
> > I'd suggest to move the statement preparation call in the 
> > CSTATE_CHOOSE_SCRIPT case.
> 
> I thought so at first, but I noticed we cannot do it at least if we are
> using -C because the connection may not be established in the
> CSTATE_CHOOSE_SCRIPT state. 
> 
> > In comments: not yet -> needed.
> 
> Thanks. I'll fix it.

I attached the updated patch v2, which includes a comment fix and a TAP test.

Regards,
Yugo Nagata

-- 
Yugo NAGATA <nagata@sraoss.co.jp>