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>
