Re: pgbench - allow backslash-continuations in custom scripts

Thank you for the comment, but could you please tell me what kind
of criteria should I take to split this patch? The discussion
about splitting criteria is in the following reply (in the
sentence begins with "By the way").

At Wed, 16 Mar 2016 11:57:25 -0400, Robert Haas <robertmhaas(at)gmail(dot)com> wrote in <CA+Tgmobvu1aBRdRaKvqMVp0ifhQpgvnOEZa2Rg3AHfRWPE5-Tg(at)mail(dot)gmail(dot)com>
> On Thu, Feb 18, 2016 at 6:54 AM, Kyotaro HORIGUCHI <
> horiguchi(dot)kyotaro(at)lab(dot)ntt(dot)co(dot)jp> wrote:
>
> > It is the SQL part of old psqlscan.l but the difference between
> > them is a bit bothersome to see. I attached the diff between them
> > as "psqlscanbody.l.diff" for convenience.
> >
>
> This is a huge diff, and I don't see that you've explained the reason for
> all the changes. For example:
>
> -/*
> - * We use a stack of flex buffers to handle substitution of psql variables.
> - * Each stacked buffer contains the as-yet-unread text from one psql
> variable.
> - * When we pop the stack all the way, we resume reading from the outer
> buffer
> - * identified by scanbufhandle.
> - */
> -typedef struct StackElem
> -{
> - YY_BUFFER_STATE buf; /* flex input control structure */
> - char *bufstring; /* data actually being scanned by
> flex *
> /
> - char *origstring; /* copy of original data, if needed
> */
> - char *varname; /* name of variable providing data,
> or N
> ULL */
> - struct StackElem *next;
> -} StackElem;
>
> Perhaps we could separate this part of the code motion into its own
> preliminary patch?

The "preliminary patch" seems to mean the same thing with the
first patch in the following message.

http://www.postgresql.org/message-id/20160107.173603.31865003.horiguchi.kyotaro@lab.ntt.co.jp

The commit log says as the following.

| Subject: [PATCH 1/5] Prepare for sharing psqlscan with pgbench.
|
| Lexer is no longer compiled as a part of mainloop.c. The slash
| command lexer is brought out from the command line lexer. psql_scan
| no longer accesses directly to pset struct and VariableSpace. This
| change allows psqlscan to be used without these things.

The following two patches are the follwings.

| Subject: [PATCH 2/5] Change the access method to shell variables
|
| Access to shell variables via a callback function so that the lexer no
| longer need to be aware of VariableSpace.

| Subject: [PATCH 3/5] Detach common.c from psqlscan
|
| Call standard_strings() and psql_error() via callback functions so
| that psqlscan.l can live totally without common.c stuff. They are
| bundled up with get_variable() callback in one struct since now we
| have as many as four callback functions.

These patches are made so as to keep the compilable and workable
state of the source files. It might be a bit more readable if
unshackled from such restriction.

> I see this went to psqlscan_int.h, but there's no
> obvious reason for that particular name, and the comments don't explain it;

I assumed that is a convention of naming by looking libpq-int.h
(though it doesn't use underscore, but hyphen). But the file
needs a comment like libpq-int.h. I'll rename it and add such
comment to it.

By the way, the patch set mentioned above gives such preliminary
steps separately. Should I make the next patch set based on the
older one which is separating the preliminary steps? Or in new
splitting criteria that is free from the compilable-workable
restriction is preferable?

> in fact, they say that's psqlscan.h. psqlscan_slash.h has the same
> problem; perhaps moving things there could be another preliminary patch.

It is also included in the 0001 patch.

> - yyless(0);
> + my_yyless(0);
>
> Why do we need to do this? Is "my_" really the best prefix? Is this
> another change that could be its own patch?

Oops! Sorry for the silly name. I was not able to think up a
proper name for it. Does psqlscan_yyless seems good?

regards,

--
Kyotaro Horiguchi
NTT Open Source Software Center