Re: [HACKERS] pgbench - allow to store select results into variables

Hello Alvaro,

> I revised this patch a bit. Here's v25, where some finishing touches
> are needed -- see below. I think with these changes the patch would
> become committable, at least for me.

Thanks a lot for having a look a this patch, and improving it.

The updated version did not work, though, but the fix was easy.

I do not understand why you have removed the num_commands stuff, which
indeed is not very useful but could be for debugging. No big deal.

> I didn't like that you were adding an #include of psqlscan_int.h into
> pgbench.c, when there's a specific comment in the header saying not to
> do that,

Oops, I did not notice the comment.

> so I opted for adding a new accessor function on psqlscan.h.

Ok.

> I renamed some of your parameter additions. I think the new names are
> clearer, but they meant the +1's in your code are now in illogical
> places. (I moved some; probably not enough). Please review/fix that.

It needed some fixing. I understood that you suggest to avoid keeping the
last index and prefer the number of elements instead, so I applied it to
the Command struct as well.

> I think "gset" is not a great name for the new struct member;

Indeed.

> please find a better name. I suggest "targetvar" but feel free to
> choose a name that suits your taste.

Ok. Note that it is not a variable name but a prefix, so I named it
"prefix".

> There are two XXX comments. One is about a function needing a comment
> atop it.

Ok.

> The other is about realloc behavior. To fix this one I would add a new
> struct member indicating the allocated size of the array, then growing
> exponentially instead of one at a time. For most cases you can probably
> get away with never reallocating beyond an initial allocation of, say, 8
> members.

Yep. I guess I did it 1 by 1 because it should be a rare case and it was
saving a counter. I've done some exponential thing instead.

> In the docs, the [prefix] part needs to be explained in the \cset entry;
> right now it's in \gset, which comes afterwards. Let's move the
> explanation up, and then in \gset say "prefix behaves as in \cset".

I do not understand, the very same explanation text about prefix appears
in both entry? The examples are different, is that what you mean? They are
about different backslash commands, so they have an interest of their own.

Attached a v26 with I hope everything ok, but for the documentation that
I'm unsure how to improve.

--
Fabien.