Re: pgbench - allow backslash-continuations in custom scripts

I'm terribly sorry to have overlooked Febien's comment.

I'll rework on this considering Febien's previous comment and
Michael's this comment in the next CF.


At Tue, 22 Dec 2015 16:17:07 +0900, Michael Paquier <michael.paquier@gmail.com> wrote in
<CAB7nPqRX_-VymeEH-3ChoPrQLgKh=EGgQ2GUtZ53ccO9uLGmxA@mail.gmail.com>
> On Wed, Oct 14, 2015 at 5:49 PM, Fabien COELHO <coelho@cri.ensmp.fr> wrote:
> >
> > Hello,
> >
> > Here is a review, sorry for the delay...
> >
> >> This is done as the additional fourth patch, not merged into
> >> previous ones, to show what's changed in the manner of command
> >> storing.
> >> [...]
> >>>
> >>>  - SQL multi-statement.
> >>>
> >>>    SELECT 1; SELECT 2;
> >
> >
> > I think this is really "SELECT 1\; SELECT 2;"

Mmm. It is differenct from my recognition. I'll confirm that.

> > I join a test script I used.

Thank you. I'll work on with it.

> > The purpose of this 4 parts patch is to reuse psql scanner from pgbench
> > so that commands are cleanly separated by ";", including managing dollar
> > quoting, having \ continuations in backslash-commands, having
> > multi-statement commands...
> >
> > This review is about 4 part v4 of the patch. The patches apply and compile
> > cleanly.
> >
> > I think that the features are worthwhile. I would have prefer more limited
> > changes to get them, but my earlier attempt was rejected, and the scanner
> > sharing with psql results in reasonably limited changes, so I would go for
> > it.
> 
> Regarding that:
> +#if !defined OUTSIDE_PSQL
> +#include "variables.h"
> +#else
> +typedef int * VariableSpace;
> +#endif
> 
> And that:
> +/* Provide dummy macros when no use of psql variables */
> +#if defined OUTSIDE_PSQL
> +#define GetVariable(space,name) NULL
> +#define standard_strings() true
> +#define psql_error(fmt,...) do { \
> +    fprintf(stderr, "psql_error is called. abort.\n");\
> +    exit(1);\
> +} while(0)
> +#endif
> That's ugly...

I have to admit that I think just the same.

> Wouldn't it be better with something say in src/common
> which is frontend-only? We could start with a set of routines allowing
> commands to be parsed. That gives us more room for future improvement.

If I read you correctly, I should cut it out into a new file and
include it. Is it correct?

> +    # fix up pg_xlogdump once it's been set up
> +    # files symlinked on Unix are copied on windows
> +    my $pgbench = AddSimpleFrontend('pgbench');
> +    $pgbench->AddDefine('FRONTEND');
> +    $pgbench->AddDefine('OUTSIDE_PSQL');
> +    $pgbench->AddFile('src/bin/psql/psqlscan.l');
> +    $pgbench->AddIncludeDir('src/bin/psql');
> This is a simple copy-paste, with an incorrect comment at least
> (haven't tested compilation with MSVC, I suspect that this is going to
> fail still the flags are correctly set).

Oops. Thank you for pointing out. It worked for me but, honestly
saying, I couldn't another clean way to do that but I'll
reconsider it.

> This patch is waiting for input from its author for quite some time
> now, and the structure of this patch needs a rework. Are folks on this
> thread fine if it is returned with feedback?

It's fine for me.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center