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.steh...@gmail.com) wrote: > 2017-01-09 17:24 GMT+01:00 Jason O'Donnell <odonnellj...@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
signature.asc
Description: Digital signature
