Pavel,
I started looking through this to see if it might be ready to commit and
I don't believe it is. Below are my comments about the first patch, I
didn't get to the point of looking at the others yet since this one had
issues.
* Pavel Stehule (pavel.stehule@gmail.com) wrote:
> 2017-01-09 17:24 GMT+01:00 Jason O'Donnell <odonnelljp01@gmail.com>:
> > gstore/gbstore:
I don't see the point to 'gstore'- how is that usefully different from
just using '\g'? Also, the comments around these are inconsistent, some
say they can only be used with a filename, others say it could be a
filename or a pipe+command.
There's a whitespace-only hunk that shouldn't be included.
I don't agree with the single-column/single-row restriction on these. I
can certainly see a case where someone might want to, say, dump out a
bunch of binary integers into a file for later processing.
The tab-completion for 'gstore' wasn't correct (you didn't include the
double-backslash). The patch also has conflicts against current master
now.
I guess my thinking about moving this forward would be to simplify it to
just '\gb' which will pull the data from the server side in binary
format and dump it out to the filename or command given. If there's a
new patch with those changes, I'll try to find time to look at it.
I would recommend going through a detailed review of the other patches
also before rebasing and re-submitting them also, in particular look to
make sure that the comments are correct and consistent, that there are
comments where there should be (generally speaking, whole functions
should have at least some comments in them, not just the function header
comment, etc).
Lastly, I'd suggest creating a 'psql.source' file for the regression
tests instead of just throwing things into 'misc.source'. Seems like we
should probably have more psql-related testing anyway and dumping
everything into 'misc.source' really isn't a good idea.
Thanks!
Stephen
