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.
\gstore ensure dump row data. It can be replaced by \g with some other setting, but if query is not unique, then the result can be messy. What is not possible with \gbstore.
More interesting is \gbstore that uses binary API - it can be used for bytea fields or for XML fields with implicit correct encoding change. \gbstore is not possible to replace by \g.
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.
ok I'll prepare patch
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.
