Re: pgbench - allow backslash-continuations in custom scripts - Mailing list pgsql-hackers

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;"

I join a test script I used.


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.


* 0001 patch about psql scanner reworking

The relevant features lexer which can be reused by pgbench are isolated
and adapted thanks to ifdefs, guards, and putting some stuff in the
current state.

I'm not sure of the "OUTSIDE_PSQL" macro name. ISTM that "PGBENCH_SCANNER"
would be better, as it makes it very clear that it is being used for pgbench,
and if someone wants to use it for something else they should define and handle
their own case explicitely.

Use "void *" instead of "int *" for VariableSpace?

Rule ":{variable_char}+": the ECHO works more or less as a side effect,
and most of the code is really ignored by pgbench. Instead of the different
changes which rely on NULL values, what about a simple ifdef/else/endif
to replace the whole stuff by ECHO for pgbench, without changing the current
code?

Also, on the same part of the code, I'm not sure about having two assignments
on the "const char * value" variable, because of "const".

The "db" parameter is not used by psql_scan_setup, so the state's db field
is never initialized, so probably "psql" does not work properly because
it seems used in several places.

I'm not sure what would happen if there are reconnections from psql (is 
that possible? Without reseting the scanner state?), as there are two 
connections, one in pset and one in the scanner state?

Variable lookup guards: why is a database connection necessary for doing
":variables" lookups? It seemed not to be required in the initial version,
and is not really needed.

Avoid changing style without clear motivation, for instance the 
PQerrorMessage/psql_error on escape_value==NULL?


*** 0002 patch to use the scanner in pgbench

There is no documentation nor examples about the new features.
I think that the internal builtin commands and the documentation should
be used to show the new features where appropriate, and insist on that
";" are now required at the end of sql commands.

If the file starts with a -- comment followed by a backslash-command, eg:  -- there is only one foo currently available
\set foo 1
 
an error is reported: the comment should probably just be ignored.

I'm not sure that the special "next" command parsing management is useful.
I do not see a significant added value to managing especially a list of
commands for commands which happened to be on the same line but separated
by a simple ";". Could not the code be simplified by just restarting
the scanner where it left, instead of looping in "process_commands"?

It seems that part of the changes are just reindentations, especially
all the parsing code for backslash commands. This should be avoided if
possible.

Some spurious spaces after "next_command:".


*** 0003 patch for ms build

I don't do windows:-)

The perl script changes look pretty reasonable, although the copied
comments refer to pg_xlogdump, I guess it should rather refer to pgbench.


*** 0004 command list patch

This patch changes the command array to use a linked list.

As the command number is needed in several places and has to be replaced by a
function call which scans the list, I do not think it is a good idea, and
I recommand not to consider this part for inclusion.

-- 
Fabien.