Re: [HACKERS] generic copy options
Tom Lane writes: > Applied with revisions as discussed. Excellent ;) Now if you wanted a small option to play with to test the extensibility of the new system, should I propose DEFAULT '\D' (e.g.)? Regards, -- dim -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Emmanuel Cecchet writes: > [ generic copy options patch ] Applied with revisions as discussed. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
On Mon, Sep 21, 2009 at 1:51 PM, Tom Lane wrote: > I wrote: >> As far as I can tell, the majority opinion is to use "format csv" > > BTW, if we're going to do that, shouldn't the "binary" option instead > be spelled "format binary"? Good catch, +1. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Tom Lane wrote: I wrote: As far as I can tell, the majority opinion is to use "format csv" BTW, if we're going to do that, shouldn't the "binary" option instead be spelled "format binary"? Looking at the doc, it looks like FORMAT should be mandatory and be either text, binary or csv (for now). manu -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
I wrote: > As far as I can tell, the majority opinion is to use "format csv" BTW, if we're going to do that, shouldn't the "binary" option instead be spelled "format binary"? regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Emmanuel Cecchet writes: > The easiest for both implementation and documentation might just be to > have a matrix of options. > Each option has a row and a column in the matrix. The intersection of a > row and a column is set to 0 if options are not compatible and set to 1 > if it is. This way we are sure to capture all possible combinations. > This way, each time we find a new option, we just have to check in the > matrix if it is compatible with the already existing options. Note that > we can also replace the 0 with an index in an error message array. This seems like overkill at the moment. Maybe when/if we get to actually supporting three or more COPY formats, we'd need it. Right now all we are trying to do is make the grammar not be a factor in adding options, and the foreseen new options aren't about new formats at all. So I'm inclined to just fix the grammar and not do any refactoring of the code in copy.c. As far as I can tell, the majority opinion is to use "format csv" and not have the "csv_" prefixes on the options, so I will adjust the patch accordingly and commit it (barring any other problems coming up when I read it more closely). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
I think that it is a good idea, but do you can show to us what do you have in mind with a example? Regards "The hurry is enemy of the success: for that reason...Be patient" Ing. Marcos L. Ortiz Valmaseda Línea Soporte y Despliegue Centro de Tecnologías de Almacenamiento y Análisis de Datos (CENTALAD) Linux User # 418229 PostgreSQL User http://www.postgresql.org http://www.planetpostgresql.org/ http://www.postgresql-es.org/ - Mensaje original - De: "Emmanuel Cecchet" Para: "Robert Haas" CC: "Emmanuel Cecchet" , "Tom Lane" , "Emmanuel Cecchet" , "Josh Berkus" , "PostgreSQL-development" Enviados: Domingo, 20 de Septiembre 2009 16:24:28 GMT -10:00 Hawai Asunto: Re: [HACKERS] generic copy options The easiest for both implementation and documentation might just be to have a matrix of options. Each option has a row and a column in the matrix. The intersection of a row and a column is set to 0 if options are not compatible and set to 1 if it is. This way we are sure to capture all possible combinations. This way, each time we find a new option, we just have to check in the matrix if it is compatible with the already existing options. Note that we can also replace the 0 with an index in an error message array. I can provide an implementation of that if this looks interesting to anyone. Emmanuel Robert Haas wrote: > On Sun, Sep 20, 2009 at 2:25 PM, Emmanuel Cecchet wrote: > >> Tom Lane wrote: >> >>> Emmanuel Cecchet writes: >>> >>>> Here you will force every format to use the same set of options >>>> >>> How does this "force" any such thing? >>> >>> >> As far as I understand it, every format will have to handle every format >> options that may exist so that they can either implement it or throw an >> error. >> > > I don't think this is really true. To be honest with you, I think > it's exactly backwards. The way the option-parsing logic works, we > parse each option individually FIRST. Then at the end we do > cross-checks to see whether there is an incompatibility in the > combination specified. So if two different formats support the same > option, we just change the cross-check to say that foo is OK with > either format bar or format baz. On the other hand, if we split the > option into bar_foo and baz_foo, then the first loop that does the > initial parsing has to support both cases, and then you still need a > separate cross-check for each one. > > >> That would argue in favor of a format option that defines the format. Right >> now I find it bogus to have to say (csv on, csv_header on). If csv_header is >> on that should imply csv on. >> The only problem I have is that it is not obvious what options are generic >> COPY options and what are options of an option (like format options). >> So maybe a tradeoff is to differentiate format specific options like in: >> (delimiter '.', format csv, format_header, format_escape...) >> This should also make clear if someone develops a new format what options >> need to be addressed. >> > > I think this is a false dichotomy. It isn't necessarily the case that > every format will support a delimiter option either. For example, if > we were to add an XML or JSON format (which I'm not at all convinced > is a good idea, but I'm sure someone is going to propose it!) it > certainly won't support specifying an arbitrary delimiter. > > IOW, *every* format will have different needs and we can't necessarily > know which options will be applicable to those needs. But as long as > we agree that we won't use the same option for two different > format-specific options with wildly different semantics, I don't think > that undecorated names are going to cause us much trouble. It's also > less typing. > > >> PS: I don't know why but as I write this message I already feel that Tom >> hates this new proposal :-D >> > > I get those feeling sometimes myself. :-) Anyway, FWIW, I think Tom > has analyzed this one correctly... > > ...Robert > > -- Emmanuel Cecchet FTO @ Frog Thinker Open Source Development & Consulting -- Web: http://www.frogthinker.org email: m...@frogthinker.org Skype: emmanuel_cecchet -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
The easiest for both implementation and documentation might just be to have a matrix of options. Each option has a row and a column in the matrix. The intersection of a row and a column is set to 0 if options are not compatible and set to 1 if it is. This way we are sure to capture all possible combinations. This way, each time we find a new option, we just have to check in the matrix if it is compatible with the already existing options. Note that we can also replace the 0 with an index in an error message array. I can provide an implementation of that if this looks interesting to anyone. Emmanuel Robert Haas wrote: On Sun, Sep 20, 2009 at 2:25 PM, Emmanuel Cecchet wrote: Tom Lane wrote: Emmanuel Cecchet writes: Here you will force every format to use the same set of options How does this "force" any such thing? As far as I understand it, every format will have to handle every format options that may exist so that they can either implement it or throw an error. I don't think this is really true. To be honest with you, I think it's exactly backwards. The way the option-parsing logic works, we parse each option individually FIRST. Then at the end we do cross-checks to see whether there is an incompatibility in the combination specified. So if two different formats support the same option, we just change the cross-check to say that foo is OK with either format bar or format baz. On the other hand, if we split the option into bar_foo and baz_foo, then the first loop that does the initial parsing has to support both cases, and then you still need a separate cross-check for each one. That would argue in favor of a format option that defines the format. Right now I find it bogus to have to say (csv on, csv_header on). If csv_header is on that should imply csv on. The only problem I have is that it is not obvious what options are generic COPY options and what are options of an option (like format options). So maybe a tradeoff is to differentiate format specific options like in: (delimiter '.', format csv, format_header, format_escape...) This should also make clear if someone develops a new format what options need to be addressed. I think this is a false dichotomy. It isn't necessarily the case that every format will support a delimiter option either. For example, if we were to add an XML or JSON format (which I'm not at all convinced is a good idea, but I'm sure someone is going to propose it!) it certainly won't support specifying an arbitrary delimiter. IOW, *every* format will have different needs and we can't necessarily know which options will be applicable to those needs. But as long as we agree that we won't use the same option for two different format-specific options with wildly different semantics, I don't think that undecorated names are going to cause us much trouble. It's also less typing. PS: I don't know why but as I write this message I already feel that Tom hates this new proposal :-D I get those feeling sometimes myself. :-) Anyway, FWIW, I think Tom has analyzed this one correctly... ...Robert -- Emmanuel Cecchet FTO @ Frog Thinker Open Source Development & Consulting -- Web: http://www.frogthinker.org email: m...@frogthinker.org Skype: emmanuel_cecchet -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
On Sun, Sep 20, 2009 at 2:25 PM, Emmanuel Cecchet wrote: > Tom Lane wrote: >> Emmanuel Cecchet writes: >>> >>> Here you will force every format to use the same set of options >> >> How does this "force" any such thing? >> > > As far as I understand it, every format will have to handle every format > options that may exist so that they can either implement it or throw an > error. I don't think this is really true. To be honest with you, I think it's exactly backwards. The way the option-parsing logic works, we parse each option individually FIRST. Then at the end we do cross-checks to see whether there is an incompatibility in the combination specified. So if two different formats support the same option, we just change the cross-check to say that foo is OK with either format bar or format baz. On the other hand, if we split the option into bar_foo and baz_foo, then the first loop that does the initial parsing has to support both cases, and then you still need a separate cross-check for each one. > That would argue in favor of a format option that defines the format. Right > now I find it bogus to have to say (csv on, csv_header on). If csv_header is > on that should imply csv on. > The only problem I have is that it is not obvious what options are generic > COPY options and what are options of an option (like format options). > So maybe a tradeoff is to differentiate format specific options like in: > (delimiter '.', format csv, format_header, format_escape...) > This should also make clear if someone develops a new format what options > need to be addressed. I think this is a false dichotomy. It isn't necessarily the case that every format will support a delimiter option either. For example, if we were to add an XML or JSON format (which I'm not at all convinced is a good idea, but I'm sure someone is going to propose it!) it certainly won't support specifying an arbitrary delimiter. IOW, *every* format will have different needs and we can't necessarily know which options will be applicable to those needs. But as long as we agree that we won't use the same option for two different format-specific options with wildly different semantics, I don't think that undecorated names are going to cause us much trouble. It's also less typing. > PS: I don't know why but as I write this message I already feel that Tom > hates this new proposal :-D I get those feeling sometimes myself. :-) Anyway, FWIW, I think Tom has analyzed this one correctly... ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Tom Lane writes: > But anyhow I think we have run through all the arguments, and it's > time for some votes from other people. Same option names whatever the format please. We know the context is important and people will be able to understand that header does not refer exactly to the same processing when applied to XML or CSV. But still refers to if those first lines are imported too... And I think I prefer 'format csv' rather than 'csv on'. Regards, -- dim -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Emmanuel Cecchet writes: > So maybe a tradeoff is to differentiate format specific options like in: > (delimiter '.', format csv, format_header, format_escape...) > This should also make clear if someone develops a new format what > options need to be addressed. I think that distinction would exist internally. What I'm not clear on is why we would want it visible externally. But anyhow I think we have run through all the arguments, and it's time for some votes from other people. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Tom Lane wrote: Emmanuel Cecchet writes: Here you will force every format to use the same set of options How does this "force" any such thing? As far as I understand it, every format will have to handle every format options that may exist so that they can either implement it or throw an error. and if someone introduces a new option, you will have to modify all other formats to make sure they throw an error telling the user that this option is not supported. Well, if we do it your way then we will instead need a collection of code to throw errors for combinations like (xml on, csv_header on). I don't really see any improvement there. That would argue in favor of a format option that defines the format. Right now I find it bogus to have to say (csv on, csv_header on). If csv_header is on that should imply csv on. The only problem I have is that it is not obvious what options are generic COPY options and what are options of an option (like format options). So maybe a tradeoff is to differentiate format specific options like in: (delimiter '.', format csv, format_header, format_escape...) This should also make clear if someone develops a new format what options need to be addressed. Emmanuel PS: I don't know why but as I write this message I already feel that Tom hates this new proposal :-D -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Emmanuel Cecchet writes: > Here you will force every format to use the same set of options How does this "force" any such thing? > and if > someone introduces a new option, you will have to modify all other > formats to make sure they throw an error telling the user that this > option is not supported. Well, if we do it your way then we will instead need a collection of code to throw errors for combinations like (xml on, csv_header on). I don't really see any improvement there. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Tom Lane wrote: No, I don't think so. Suppose I write COPY ... (xml_header on) If HEADER isn't actually an option supported by XML format, what I will get here is an "unknown option" error, which conveys just about nothing --- is it really an unsupported combination, or did I just misspell the option name? Well, I don't see why you would write that if the option is not documented. Usually as a user, when I need to use a command, I look at the doc/man page and use the options that are indicated, I don't try to invent new options. That should prevent the kind of scenario you describe here: If we go with the other way then I would expect COPY ... (xml, header on) to draw a specific "HEADER is not supported in XML format" error. Of course, that will require some extra code to make it happen. So you could argue that format-specific option names are easier from the lazy programmer's viewpoint. But I don't believe the argument that they're better from the user's viewpoint. Here you will force every format to use the same set of options and if someone introduces a new option, you will have to modify all other formats to make sure they throw an error telling the user that this option is not supported. I don't think this is a great design and that it will be easy to extend. Emmanuel -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Emmanuel Cecchet writes: > That would assume that the semantic of the other options (header, quote, > espace, ...) is exactly the same for each format. Otherwise this will be > a nightmare to document. Well, sure, we should pick a different name for an option that means something significantly different. But for options that do mean approximately the same thing in different formats, it doesn't seem helpful to require different names to be used. > If we don't prefix with CSV, I guess that some users will spend some > time to figure out that NULL is not a format option but FORCE NOT NULL > is. If an option is only supported by one format (let's say XML) we will > have to document every time which options are supported by which format > which would be much easier and intuitive is options were readily > prefixed by the format name. No, I don't think so. Suppose I write COPY ... (xml_header on) If HEADER isn't actually an option supported by XML format, what I will get here is an "unknown option" error, which conveys just about nothing --- is it really an unsupported combination, or did I just misspell the option name? If we go with the other way then I would expect COPY ... (xml, header on) to draw a specific "HEADER is not supported in XML format" error. Of course, that will require some extra code to make it happen. So you could argue that format-specific option names are easier from the lazy programmer's viewpoint. But I don't believe the argument that they're better from the user's viewpoint. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Tom Lane wrote: Robert Haas writes: ... A related question is whether we should replace the "CSV" option with a "FORMAT" option for which one of the possible choices is "CSV" (much as we did with EXPLAIN). That might be a good idea --- otherwise we'd need some ad-hoc code to check that only one format option has been selected. On the other hand, it wouldn't be all that much code; and it would be a rather larger change from previous behavior than we were contemplating to start with. Comments anyone? That would assume that the semantic of the other options (header, quote, espace, ...) is exactly the same for each format. Otherwise this will be a nightmare to document. If we don't prefix with CSV, I guess that some users will spend some time to figure out that NULL is not a format option but FORCE NOT NULL is. If an option is only supported by one format (let's say XML) we will have to document every time which options are supported by which format which would be much easier and intuitive is options were readily prefixed by the format name. If you look at the COPY documentation in the error logging patch, if we strip the error_logging prefix, it is going to be very confusing. But I am willing to let Tom choose whatever he prefers as his birthday gift ;-) One other minor point is that the patch introduces an empty-list syntax for individual option values, but then treats it the same as specifying nothing: It seemed like a good idea because if you can do force_quote (a, b, c) and force_quote (a, b) you might think that you could also do force_quote (). Particularly for the sake of people generating SQL automatically by some means, it seems like this might simplify life. But possibly it shouldn't evaluate to the same value, so that you can't write OIDS () to mean the same thing as OIDS ON. Yeah, that type of scenario was why I didn't like it. I am not impressed by the empty-list argument, either. We don't support empty lists with that sort of syntax in most other places, so why here? There are counter-precedents even in the syntax of COPY itself: you can't write "()" for an empty column name list, and you can't write "()" for an empty copy_generic_option_list. Well this one was in Robert's initial patch and I left it as is. I don't have any strong opinion for or against it. Emmanuel -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Robert Haas writes: > ... A related question is whether we > should replace the "CSV" option with a "FORMAT" option for which one > of the possible choices is "CSV" (much as we did with EXPLAIN). That might be a good idea --- otherwise we'd need some ad-hoc code to check that only one format option has been selected. On the other hand, it wouldn't be all that much code; and it would be a rather larger change from previous behavior than we were contemplating to start with. Comments anyone? >> One other minor point is that the patch introduces an empty-list >> syntax for individual option values, but then treats it the same >> as specifying nothing: > It seemed like a good idea because if you can do force_quote (a, b, c) > and force_quote (a, b) you might think that you could also do > force_quote (). Particularly for the sake of people generating SQL > automatically by some means, it seems like this might simplify life. > But possibly it shouldn't evaluate to the same value, so that you > can't write OIDS () to mean the same thing as OIDS ON. Yeah, that type of scenario was why I didn't like it. I am not impressed by the empty-list argument, either. We don't support empty lists with that sort of syntax in most other places, so why here? There are counter-precedents even in the syntax of COPY itself: you can't write "()" for an empty column name list, and you can't write "()" for an empty copy_generic_option_list. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
On Sat, Sep 19, 2009 at 3:10 PM, Tom Lane wrote: > Emmanuel Cecchet writes: >> [ latest patch version ] > > Do we have consensus on the syntax for this patch? In particular, > what about the question of adding CSV_ to all the CSV-specific option > names? Emmanuel argued that this is necessary to avoid confusion if > we someday introduce other copy formats that have similar options. > However, I think you could easily turn that argument around. Any one > COPY command will surely use just one format, and it seems to me that > forcing different formats to use different names for equivalent options > won't simplify life for anybody. So I'm inclined to think we should > not have the CSV_ prefixes. (I seem to recall that we had exactly > this discussion when the options were introduced the first time, and > settled on not using format-specific option names.) Agreed. It doesn't seem inconceivable that some other format could have a "header" or "quote" option. A related question is whether we should replace the "CSV" option with a "FORMAT" option for which one of the possible choices is "CSV" (much as we did with EXPLAIN). > One other minor point is that the patch introduces an empty-list > syntax for individual option values, but then treats it the same > as specifying nothing: > >> + | '(' ')' { $$ = NULL; } >> + | /* EMPTY */ { $$ = NULL; } > > I'm not convinced this is a a good idea, and in any case I don't see > it documented. I'm inclined to omit the '(' ')' syntax. It seemed like a good idea because if you can do force_quote (a, b, c) and force_quote (a, b) you might think that you could also do force_quote (). Particularly for the sake of people generating SQL automatically by some means, it seems like this might simplify life. But possibly it shouldn't evaluate to the same value, so that you can't write OIDS () to mean the same thing as OIDS ON. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Emmanuel Cecchet writes: > [ generic copy options patch ] I went ahead and applied the psql \copy part of this, since that saves us a couple hundred lines of code regardless of what may or may not happen on the backend side. There were a couple of minor bugs, and I also found a few other simplifications we could make in the same area, eg if we're not going to parse the options exactly then we needn't be too picky about the column list syntax either. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Emmanuel Cecchet writes: > [ latest patch version ] Do we have consensus on the syntax for this patch? In particular, what about the question of adding CSV_ to all the CSV-specific option names? Emmanuel argued that this is necessary to avoid confusion if we someday introduce other copy formats that have similar options. However, I think you could easily turn that argument around. Any one COPY command will surely use just one format, and it seems to me that forcing different formats to use different names for equivalent options won't simplify life for anybody. So I'm inclined to think we should not have the CSV_ prefixes. (I seem to recall that we had exactly this discussion when the options were introduced the first time, and settled on not using format-specific option names.) One other minor point is that the patch introduces an empty-list syntax for individual option values, but then treats it the same as specifying nothing: > + | '(' ')'{ $$ = NULL; } > + | /* EMPTY */{ $$ = NULL; } I'm not convinced this is a a good idea, and in any case I don't see it documented. I'm inclined to omit the '(' ')' syntax. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Josh Berkus wrote: Nope, but it was on the checklist and I was being thorough. That's a good thing. I was just seeing if I needed to get involved in performance testing. That would be good to have more people test the autopartitioning feature in COPY. If you want to be involved in performance testing on that, that would be great. Emmanuel -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
On Fri, Sep 18, 2009 at 10:31:21AM -0700, Josh Berkus wrote: > > > Nope, but it was on the checklist and I was being thorough. > > That's a good thing. I was just seeing if I needed to get involved in > performance testing. > > -- > Josh Berkus > PostgreSQL Experts Inc. > www.pgexperts.com I always say, the more tests the better. From the tests I ran it was clear the parser did not change speed. Might be good to have someone confirm that. -- Dan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
> Nope, but it was on the checklist and I was being thorough. That's a good thing. I was just seeing if I needed to get involved in performance testing. -- Josh Berkus PostgreSQL Experts Inc. www.pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
On Fri, Sep 18, 2009 at 10:21:08AM -0700, Josh Berkus wrote: > On 9/17/09 3:54 PM, Greg Smith wrote: > > On Thu, 17 Sep 2009, Dan Colish wrote: > > > >> - Performance appears to be the same although I don't have a good > >> way for > >> testing this at the moment > > > > Here's what I do to generate simple COPY performance test cases: > > Is there any reason to think that *this* copy patch will affect > performance at all? > > -- > Josh Berkus > PostgreSQL Experts Inc. > www.pgexperts.com > Nope, but it was on the checklist and I was being thorough. -- Dan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
On 9/17/09 3:54 PM, Greg Smith wrote: > On Thu, 17 Sep 2009, Dan Colish wrote: > >> - Performance appears to be the same although I don't have a good >> way for >> testing this at the moment > > Here's what I do to generate simple COPY performance test cases: Is there any reason to think that *this* copy patch will affect performance at all? -- Josh Berkus PostgreSQL Experts Inc. www.pgexperts.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Robert Haas wrote: On Thu, Sep 17, 2009 at 8:31 PM, Dan Colish wrote: Ok, so I ran something like you suggested and did a simple copy from an empty file to just test the parsing. I have the COPY statement run 3733 times in the transaction block and did the select timestamps, but I still only was a few milliseconds difference between the two versions. Maybe a more complex copy statment could be a better test of the parser, but I do not see a significant difference of parsing speed here. I find that entirely unsurprising. Me too. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
On Thu, Sep 17, 2009 at 8:31 PM, Dan Colish wrote: > Ok, so I ran something like you suggested and did a simple copy from an > empty file to just test the parsing. I have the COPY statement run 3733 > times in the transaction block and did the select timestamps, but I > still only was a few milliseconds difference between the two versions. > Maybe a more complex copy statment could be a better test of the parser, > but I do not see a significant difference of parsing speed here. I find that entirely unsurprising. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
On Thu, Sep 17, 2009 at 07:45:45PM -0400, Andrew Dunstan wrote: > > > Dan Colish wrote: >> CREATE TABLE >> INSERT 0 10 >> Timing is on. >> COPY 10 >> Time: 83.273 ms >> BEGIN >> Time: 0.412 ms >> TRUNCATE TABLE >> Time: 0.357 ms >> COPY 10 >> Time: 140.911 ms >> COMMIT >> Time: 4.909 ms >> >> >> > > Anything that doesn't have times that are orders of magnitude greater > than this is pretty much useless as a measurement of COPY performance, > IMNSHO. > > In this particular test, to check for paring times, I'd be inclined to > do copy repeatedly (i.e. probably quite a few thousand times) from an > empty file to test the speed. Something like: > >select current_timestamp; >begin; >truncate; >copy;copy;copy; ... >commit; >select current_timestamp; > > > (tests like this are really a good case for DO ' something'; - we could > put a loop in the DO.) > > cheers > > andrew > Ok, so I ran something like you suggested and did a simple copy from an empty file to just test the parsing. I have the COPY statement run 3733 times in the transaction block and did the select timestamps, but I still only was a few milliseconds difference between the two versions. Maybe a more complex copy statment could be a better test of the parser, but I do not see a significant difference of parsing speed here. -- --Dan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Dan Colish wrote: CREATE TABLE INSERT 0 10 Timing is on. COPY 10 Time: 83.273 ms BEGIN Time: 0.412 ms TRUNCATE TABLE Time: 0.357 ms COPY 10 Time: 140.911 ms COMMIT Time: 4.909 ms Anything that doesn't have times that are orders of magnitude greater than this is pretty much useless as a measurement of COPY performance, IMNSHO. In this particular test, to check for paring times, I'd be inclined to do copy repeatedly (i.e. probably quite a few thousand times) from an empty file to test the speed. Something like: select current_timestamp; begin; truncate; copy;copy;copy; ... commit; select current_timestamp; (tests like this are really a good case for DO ' something'; - we could put a loop in the DO.) cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
On that particular patch, as Robert mentioned, only the parsing has changed. In the case of \copy, the parsing is much lighter than before in psql (remains the same in the server). The bigger the COPY operation the less you will see the impact of the parsing since it is done only once for the entire operation. Emmanuel Dan Colish wrote: On Thu, Sep 17, 2009 at 07:10:35PM -0400, Andrew Dunstan wrote: Greg Smith wrote: On Thu, 17 Sep 2009, Dan Colish wrote: - Performance appears to be the same although I don't have a good way for testing this at the moment Here's what I do to generate simple COPY performance test cases: CREATE TABLE t (i integer); INSERT INTO t SELECT x FROM generate_series(1,10) AS x; \timing COPY t TO '/some/file' WITH [options]; BEGIN; TRUNCATE TABLE t; COPY t FROM '/some/file' WITH [options]; COMMIT; You can adjust the size of the generated table based on whether you want to minimize (small number) or maximize (big number) the impact of the setup overhead relative to actual processing time. Big numbers make sense if there's a per-row change, small ones if it's mainly COPY setup that's been changed if you want a small bit of data to test against. An example with one column in it is a good test case for seeing whether per-row impact has gone up. You'd want something with a wider row for other types of performance tests. The reason for the BEGIN/COMMIT there is that form utilizes an optimization that lowers WAL volume when doing the COPY insertion, which makes it more likely you'll be testing performance of the right thing. I usually prefer to test with a table that is more varied than anything you can make with generate_series. When I tested my ragged copy patch the other day I copied 1,000,000 rows out of a large table with a mixture of dates, strings, numbers and nulls. But then, it has a (tiny) per field overhead so I wanted to make sure that was well represented in the test. You are certainly right about wrapping it in begin/truncate/commit (and when you do make sure that archiving is not on). You probably want to make sure that the file is not on the same disk as the database, to avoid disk contention. Or, better, make sure that it is in OS file system cache, or on a RAM disk. cheers andrew If someone with a more significant setup can run tests that would ideal. I only have my laptop which is a single disk and fairly underpowered. That said, here are my results running the script above, it looks like the pach improves performance. I would really interested to see results on a larger data set and heavier iron. -- --Dan Without Patch: CREATE TABLE INSERT 0 10 Timing is on. COPY 10 Time: 83.273 ms BEGIN Time: 0.412 ms TRUNCATE TABLE Time: 0.357 ms COPY 10 Time: 140.911 ms COMMIT Time: 4.909 ms With Patch: CREATE TABLE INSERT 0 10 Timing is on. COPY 10 Time: 80.205 ms BEGIN Time: 0.351 ms TRUNCATE TABLE Time: 0.346 ms COPY 10 Time: 124.303 ms COMMIT Time: 4.130 ms -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
On Thu, Sep 17, 2009 at 07:10:35PM -0400, Andrew Dunstan wrote: > > > Greg Smith wrote: >> On Thu, 17 Sep 2009, Dan Colish wrote: >> >>> - Performance appears to be the same although I don't have a good >>> way for >>> testing this at the moment >> >> Here's what I do to generate simple COPY performance test cases: >> >> CREATE TABLE t (i integer); >> INSERT INTO t SELECT x FROM generate_series(1,10) AS x; >> \timing >> COPY t TO '/some/file' WITH [options]; >> BEGIN; >> TRUNCATE TABLE t; >> COPY t FROM '/some/file' WITH [options]; >> COMMIT; >> >> You can adjust the size of the generated table based on whether you >> want to minimize (small number) or maximize (big number) the impact of >> the setup overhead relative to actual processing time. Big numbers >> make sense if there's a per-row change, small ones if it's mainly COPY >> setup that's been changed if you want a small bit of data to test >> against. >> >> An example with one column in it is a good test case for seeing >> whether per-row impact has gone up. You'd want something with a wider >> row for other types of performance tests. >> >> The reason for the BEGIN/COMMIT there is that form utilizes an >> optimization that lowers WAL volume when doing the COPY insertion, >> which makes it more likely you'll be testing performance of the right >> thing. >> >> > > I usually prefer to test with a table that is more varied than anything > you can make with generate_series. When I tested my ragged copy patch > the other day I copied 1,000,000 rows out of a large table with a > mixture of dates, strings, numbers and nulls. > > But then, it has a (tiny) per field overhead so I wanted to make sure > that was well represented in the test. > > You are certainly right about wrapping it in begin/truncate/commit (and > when you do make sure that archiving is not on). > > You probably want to make sure that the file is not on the same disk as > the database, to avoid disk contention. Or, better, make sure that it is > in OS file system cache, or on a RAM disk. > > cheers > > andrew If someone with a more significant setup can run tests that would ideal. I only have my laptop which is a single disk and fairly underpowered. That said, here are my results running the script above, it looks like the pach improves performance. I would really interested to see results on a larger data set and heavier iron. -- --Dan Without Patch: CREATE TABLE INSERT 0 10 Timing is on. COPY 10 Time: 83.273 ms BEGIN Time: 0.412 ms TRUNCATE TABLE Time: 0.357 ms COPY 10 Time: 140.911 ms COMMIT Time: 4.909 ms With Patch: CREATE TABLE INSERT 0 10 Timing is on. COPY 10 Time: 80.205 ms BEGIN Time: 0.351 ms TRUNCATE TABLE Time: 0.346 ms COPY 10 Time: 124.303 ms COMMIT Time: 4.130 ms -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Greg Smith wrote: On Thu, 17 Sep 2009, Dan Colish wrote: - Performance appears to be the same although I don't have a good way for testing this at the moment Here's what I do to generate simple COPY performance test cases: CREATE TABLE t (i integer); INSERT INTO t SELECT x FROM generate_series(1,10) AS x; \timing COPY t TO '/some/file' WITH [options]; BEGIN; TRUNCATE TABLE t; COPY t FROM '/some/file' WITH [options]; COMMIT; You can adjust the size of the generated table based on whether you want to minimize (small number) or maximize (big number) the impact of the setup overhead relative to actual processing time. Big numbers make sense if there's a per-row change, small ones if it's mainly COPY setup that's been changed if you want a small bit of data to test against. An example with one column in it is a good test case for seeing whether per-row impact has gone up. You'd want something with a wider row for other types of performance tests. The reason for the BEGIN/COMMIT there is that form utilizes an optimization that lowers WAL volume when doing the COPY insertion, which makes it more likely you'll be testing performance of the right thing. I usually prefer to test with a table that is more varied than anything you can make with generate_series. When I tested my ragged copy patch the other day I copied 1,000,000 rows out of a large table with a mixture of dates, strings, numbers and nulls. But then, it has a (tiny) per field overhead so I wanted to make sure that was well represented in the test. You are certainly right about wrapping it in begin/truncate/commit (and when you do make sure that archiving is not on). You probably want to make sure that the file is not on the same disk as the database, to avoid disk contention. Or, better, make sure that it is in OS file system cache, or on a RAM disk. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
On Thu, Sep 17, 2009 at 6:54 PM, Greg Smith wrote: > On Thu, 17 Sep 2009, Dan Colish wrote: > >> - Performance appears to be the same although I don't have a good >> way for >> testing this at the moment > > Here's what I do to generate simple COPY performance test cases: > > CREATE TABLE t (i integer); > INSERT INTO t SELECT x FROM generate_series(1,10) AS x; > \timing > COPY t TO '/some/file' WITH [options]; > BEGIN; > TRUNCATE TABLE t; > COPY t FROM '/some/file' WITH [options]; > COMMIT; > > You can adjust the size of the generated table based on whether you want to > minimize (small number) or maximize (big number) the impact of the setup > overhead relative to actual processing time. Big numbers make sense if > there's a per-row change, small ones if it's mainly COPY setup that's been > changed if you want a small bit of data to test against. > > An example with one column in it is a good test case for seeing whether > per-row impact has gone up. You'd want something with a wider row for other > types of performance tests. > > The reason for the BEGIN/COMMIT there is that form utilizes an optimization > that lowers WAL volume when doing the COPY insertion, which makes it more > likely you'll be testing performance of the right thing. Unless something has changed drastically in the last day or two, this patch is only affecting the option-parsing phase of copy, so the impact should be nearly all but noticeable, and it should be an up-front cost, not per row. It would be good to verify that, of course. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
On Thu, 17 Sep 2009, Dan Colish wrote: - Performance appears to be the same although I don't have a good way for testing this at the moment Here's what I do to generate simple COPY performance test cases: CREATE TABLE t (i integer); INSERT INTO t SELECT x FROM generate_series(1,10) AS x; \timing COPY t TO '/some/file' WITH [options]; BEGIN; TRUNCATE TABLE t; COPY t FROM '/some/file' WITH [options]; COMMIT; You can adjust the size of the generated table based on whether you want to minimize (small number) or maximize (big number) the impact of the setup overhead relative to actual processing time. Big numbers make sense if there's a per-row change, small ones if it's mainly COPY setup that's been changed if you want a small bit of data to test against. An example with one column in it is a good test case for seeing whether per-row impact has gone up. You'd want something with a wider row for other types of performance tests. The reason for the BEGIN/COMMIT there is that form utilizes an optimization that lowers WAL volume when doing the COPY insertion, which makes it more likely you'll be testing performance of the right thing. -- * Greg Smith gsm...@gregsmith.com http://www.gregsmith.com Baltimore, MD -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Hi, I have read through the patch a few times and it looks OK. The additions to the COPY syntax work as expected and as agreed upon based on the thread. Below are some points from my checklist. - Patch applies cleanly - Included new tests and documentation - Well commented - Documentation is clearly written - Produced no error or warning on compile - When compiled passes all tests - Syntax works as expected - Performance appears to be the same although I don't have a good way for testing this at the moment - Patch integrates well with current backend copy functions - Patch cleanly extends the psql \copy feature Any further thoughts on this patch? I think its pretty much ready. -- --Dan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Here you go. Emmanuel Dan Colish wrote: On Thu, Sep 17, 2009 at 02:56:07PM -0400, Emmanuel Cecchet wrote: Dan, My bad, I copy/pasted the hard link in output/copy.source instead of @abs_build...@. Here is a complete version of the patch with the fix on output/copy.source. Emmanuel Emmanuel, Thanks for getting that back so quickly. As I said before, it applies cleanly and passes regression tests. I'm reading through the changes now. When you get a moment could you send me the patch as a context diff? -- --Dan -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com Index: src/test/regress/sql/copy2.sql === RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/sql/copy2.sql,v retrieving revision 1.18 diff -c -r1.18 copy2.sql *** src/test/regress/sql/copy2.sql 25 Jul 2009 00:07:14 - 1.18 --- src/test/regress/sql/copy2.sql 17 Sep 2009 20:59:50 - *** *** 73,89 \. -- various COPY options: delimiters, oids, NULL string ! COPY x (b, c, d, e) from stdin with oids delimiter ',' null 'x'; 50,x,45,80,90 51,x,\x,\\x,\\\x 52,x,\,,\\\,,\\ \. ! COPY x from stdin WITH DELIMITER AS ';' NULL AS ''; 3000;;c;; \. ! COPY x from stdin WITH DELIMITER AS ':' NULL AS E'\\X'; 4000:\X:C:\X:\X 4001:1:empty:: 4002:2:null:\X:\X --- 73,89 \. -- various COPY options: delimiters, oids, NULL string ! COPY x (b, c, d, e) from stdin (oids, delimiter ',', null 'x'); 50,x,45,80,90 51,x,\x,\\x,\\\x 52,x,\,,\\\,,\\ \. ! COPY x from stdin (DELIMITER ';', NULL ''); 3000;;c;; \. ! COPY x from stdin (DELIMITER ':', NULL E'\\X'); 4000:\X:C:\X:\X 4001:1:empty:: 4002:2:null:\X:\X *** *** 108,120 INSERT INTO no_oids (a, b) VALUES (20, 30); -- should fail ! COPY no_oids FROM stdin WITH OIDS; ! COPY no_oids TO stdout WITH OIDS; -- check copy out COPY x TO stdout; COPY x (c, e) TO stdout; ! COPY x (b, e) TO stdout WITH NULL 'I''m null'; CREATE TEMP TABLE y ( col1 text, --- 108,120 INSERT INTO no_oids (a, b) VALUES (20, 30); -- should fail ! COPY no_oids FROM stdin (OIDS); ! COPY no_oids TO stdout (OIDS); -- check copy out COPY x TO stdout; COPY x (c, e) TO stdout; ! COPY x (b, e) TO stdout (NULL 'I''m null'); CREATE TEMP TABLE y ( col1 text, *** *** 130,140 COPY y TO stdout WITH CSV FORCE QUOTE col2 ESCAPE E'\\'; COPY y TO stdout WITH CSV FORCE QUOTE *; --test that we read consecutive LFs properly CREATE TEMP TABLE testnl (a int, b text, c int); ! COPY testnl FROM stdin CSV; 1,"a field with two LFs inside",2 --- 130,152 COPY y TO stdout WITH CSV FORCE QUOTE col2 ESCAPE E'\\'; COPY y TO stdout WITH CSV FORCE QUOTE *; + -- Test new 8.5 syntax + + COPY y TO stdout (CSV); + COPY y TO stdout (CSV, CSV_QUOTE , DELIMITER '|'); + COPY y TO stdout (CSV, CSV_FORCE_QUOTE (col2), CSV_ESCAPE E'\\'); + COPY y TO stdout (CSV, CSV_FORCE_QUOTE *); + + \COPY y TO stdout (CSV) + \COPY y TO stdout (CSV, CSV_QUOTE , DELIMITER '|') + \COPY y TO stdout (CSV, CSV_FORCE_QUOTE (col2), CSV_ESCAPE E'\\') + \COPY y TO stdout (CSV, CSV_FORCE_QUOTE *) + --test that we read consecutive LFs properly CREATE TEMP TABLE testnl (a int, b text, c int); ! COPY testnl FROM stdin (CSV); 1,"a field with two LFs inside",2 *** *** 143,156 -- test end of copy marker CREATE TEMP TABLE testeoc (a text); ! COPY testeoc FROM stdin CSV; a\. \.b c\.d "\." \. ! COPY testeoc TO stdout CSV; DROP TABLE x, y; DROP FUNCTION fn_x_before(); --- 155,168 -- test end of copy marker CREATE TEMP TABLE testeoc (a text); ! COPY testeoc FROM stdin (CSV); a\. \.b c\.d "\." \. ! COPY testeoc TO stdout (CSV); DROP TABLE x, y; DROP FUNCTION fn_x_before(); Index: src/test/regress/sql/aggregates.sql === RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/sql/aggregates.sql,v retrieving revision 1.15 diff -c -r1.15 aggregates.sql *** src/test/regress/sql/aggregates.sql 25 Apr 2009 16:44:56 - 1.15 --- src/test/regress/sql/aggregates.sql 17 Sep 2009 20:59:50 - *** *** 104,110 BIT_OR(i4) AS "?" FROM bitwise_test; ! COPY bitwise_test FROM STDIN NULL 'null'; 1 1 1 1 1 B0101 3 3 3 null2 B0100 7 7 7 3 4 B1100 --- 104,110 BIT_OR(i4) AS "?" FROM bitwise_test; ! COPY bitwise_test FROM STDIN (NULL 'null'); 1 1 1 1 1 B0101 3 3 3 null2 B0100 7 7 7 3 4 B1100 *** *** 171,177 BOOL_OR(b3)AS "n" FROM bool_test; ! COPY bool_test FROM STDIN NULL '
Re: [HACKERS] generic copy options
On Thu, Sep 17, 2009 at 02:56:07PM -0400, Emmanuel Cecchet wrote: > Dan, > > My bad, I copy/pasted the hard link in output/copy.source instead of > @abs_build...@. > Here is a complete version of the patch with the fix on output/copy.source. > > Emmanuel Emmanuel, Thanks for getting that back so quickly. As I said before, it applies cleanly and passes regression tests. I'm reading through the changes now. When you get a moment could you send me the patch as a context diff? -- --Dan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
On Thu, Sep 17, 2009 at 02:56:07PM -0400, Emmanuel Cecchet wrote: > Dan, > > My bad, I copy/pasted the hard link in output/copy.source instead of > @abs_build...@. > Here is a complete version of the patch with the fix on output/copy.source. > > Emmanuel > Hooray, that works just fine now. I guess I should have seen that... -- --Dan -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Dan, My bad, I copy/pasted the hard link in output/copy.source instead of @abs_build...@. Here is a complete version of the patch with the fix on output/copy.source. Emmanuel On Thu, Sep 17, 2009 at 11:07:33AM -0400, Emmanuel Cecchet wrote: Tom Lane wrote: I wonder though if we couldn't simplify matters. Offhand it seems to me that psql doesn't need to validate the command's syntax fully. All it really needs to do is find the target filename and replace it with STDIN/STDOUT. Could we have it just treat the remainder of the line literally, and not worry about the details of what the options might be? Let the backend worry about throwing an error if they're bad. New version with the simplified psql. Still supports the 7.3 syntax. I am going to look into porting the other COPY enhancements (error logging and autopartitioning) on this implementation. We might come up with new ideas for the documentation side of things with more options. manu -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com Hi, I've been working on a review of this patch and currently its failing regression tests. Here's the regression.diff. I'm going to spend some time today trying to figure out if the tests need to change of there is an actual issue in the patch. Just wanted to give a heads up. -- --Dan *** /home/dc0lish/workspace/postgresql/src/test/regress/expected/copy.out 2009-09-17 11:45:04.041818319 -0700 --- /home/dc0lish/workspace/postgresql/src/test/regress/results/copy.out 2009-09-17 11:45:14.215152558 -0700 *** *** 72,88 1,a,1 2,b,2 -- Repeat the above tests with the new 8.5 option syntax ! copy copytest to '/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' (csv); truncate copytest2; ! copy copytest2 from '/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' (csv); select * from copytest except select * from copytest2; style | test | filler ---+--+ (0 rows) truncate copytest2; ! copy copytest to '/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' (csv, csv_quote , csv_escape E'\\'); ! copy copytest2 from '/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' (csv, csv_quote , csv_escape E'\\'); select * from copytest except select * from copytest2; style | test | filler ---+--+ --- 72,88 1,a,1 2,b,2 -- Repeat the above tests with the new 8.5 option syntax ! copy copytest to '/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv); truncate copytest2; ! copy copytest2 from '/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv); select * from copytest except select * from copytest2; style | test | filler ---+--+ (0 rows) truncate copytest2; ! copy copytest to '/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv, csv_quote , csv_escape E'\\'); ! copy copytest2 from '/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv, csv_quote , csv_escape E'\\'); select * from copytest except select * from copytest2; style | test | filler ---+--+ *** *** 95,111 1,a,1 2,b,2 -- Repeat the above tests with the new 8.5 option syntax from psql ! \copy copytest to '/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' (csv) truncate copytest2; ! \copy copytest2 from '/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' (csv) select * from copytest except select * from copytest2; style | test | filler ---+--+ (0 rows) truncate copytest2; ! \copy copytest to '/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' (csv, csv_quote , csv_escape E'\\') ! \copy copytest2 from '/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' (csv, csv_quote , csv_escape E'\\') select * from copytest except select * from copytest2; style | test | filler ---+--+ --- 95,111 1,a,1 2,b,2 -- Repeat the above tests with the new 8.5 option syntax from psql ! \copy copytest to '/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv) truncate copytest2; ! \copy copytest2 from '/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv) select * from copytest except select * from copytest2; style | test | filler ---+--+ (0 rows) truncate copytest2; ! \copy copytest to '/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv, csv_quote , csv_escape E'\\') ! \copy copytest2 from '/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv, csv_quote , c
Re: [HACKERS] generic copy options
On Thu, Sep 17, 2009 at 11:07:33AM -0400, Emmanuel Cecchet wrote: > Tom Lane wrote: >> I wonder though if we couldn't simplify matters. Offhand it seems to me >> that psql doesn't need to validate the command's syntax fully. All it >> really needs to do is find the target filename and replace it with >> STDIN/STDOUT. Could we have it just treat the remainder of the line >> literally, and not worry about the details of what the options might be? >> Let the backend worry about throwing an error if they're bad. >> > New version with the simplified psql. Still supports the 7.3 syntax. > I am going to look into porting the other COPY enhancements (error > logging and autopartitioning) on this implementation. We might come up > with new ideas for the documentation side of things with more options. > > manu > > -- > Emmanuel Cecchet > Aster Data Systems > Web: http://www.asterdata.com > Hi, I've been working on a review of this patch and currently its failing regression tests. Here's the regression.diff. I'm going to spend some time today trying to figure out if the tests need to change of there is an actual issue in the patch. Just wanted to give a heads up. -- --Dan *** /home/dc0lish/workspace/postgresql/src/test/regress/expected/copy.out 2009-09-17 11:45:04.041818319 -0700 --- /home/dc0lish/workspace/postgresql/src/test/regress/results/copy.out 2009-09-17 11:45:14.215152558 -0700 *** *** 72,88 1,a,1 2,b,2 -- Repeat the above tests with the new 8.5 option syntax ! copy copytest to '/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' (csv); truncate copytest2; ! copy copytest2 from '/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' (csv); select * from copytest except select * from copytest2; style | test | filler ---+--+ (0 rows) truncate copytest2; ! copy copytest to '/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' (csv, csv_quote , csv_escape E'\\'); ! copy copytest2 from '/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' (csv, csv_quote , csv_escape E'\\'); select * from copytest except select * from copytest2; style | test | filler ---+--+ --- 72,88 1,a,1 2,b,2 -- Repeat the above tests with the new 8.5 option syntax ! copy copytest to '/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv); truncate copytest2; ! copy copytest2 from '/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv); select * from copytest except select * from copytest2; style | test | filler ---+--+ (0 rows) truncate copytest2; ! copy copytest to '/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv, csv_quote , csv_escape E'\\'); ! copy copytest2 from '/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv, csv_quote , csv_escape E'\\'); select * from copytest except select * from copytest2; style | test | filler ---+--+ *** *** 95,111 1,a,1 2,b,2 -- Repeat the above tests with the new 8.5 option syntax from psql ! \copy copytest to '/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' (csv) truncate copytest2; ! \copy copytest2 from '/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' (csv) select * from copytest except select * from copytest2; style | test | filler ---+--+ (0 rows) truncate copytest2; ! \copy copytest to '/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' (csv, csv_quote , csv_escape E'\\') ! \copy copytest2 from '/home/manu/workspace/Postgres8.5-COPY/src/test/regress/results/copytest.csv' (csv, csv_quote , csv_escape E'\\') select * from copytest except select * from copytest2; style | test | filler ---+--+ --- 95,111 1,a,1 2,b,2 -- Repeat the above tests with the new 8.5 option syntax from psql ! \copy copytest to '/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv) truncate copytest2; ! \copy copytest2 from '/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv) select * from copytest except select * from copytest2; style | test | filler ---+--+ (0 rows) truncate copytest2; ! \copy copytest to '/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv, csv_quote , csv_escape E'\\') ! \copy copytest2 from '/home/dc0lish/workspace/postgresql/src/test/regress/results/copytest.csv' (csv, csv_quote , csv_escape E'\\') select * from copytest except select * from copytest2; style | test | filler ---+--+ ==
Re: [HACKERS] generic copy options
Tom Lane wrote: I wonder though if we couldn't simplify matters. Offhand it seems to me that psql doesn't need to validate the command's syntax fully. All it really needs to do is find the target filename and replace it with STDIN/STDOUT. Could we have it just treat the remainder of the line literally, and not worry about the details of what the options might be? Let the backend worry about throwing an error if they're bad. New version with the simplified psql. Still supports the 7.3 syntax. I am going to look into porting the other COPY enhancements (error logging and autopartitioning) on this implementation. We might come up with new ideas for the documentation side of things with more options. manu -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com ### Eclipse Workspace Patch 1.0 #P Postgres8.5-COPY Index: src/test/regress/sql/copy2.sql === RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/sql/copy2.sql,v retrieving revision 1.18 diff -u -r1.18 copy2.sql --- src/test/regress/sql/copy2.sql 25 Jul 2009 00:07:14 - 1.18 +++ src/test/regress/sql/copy2.sql 17 Sep 2009 15:04:06 - @@ -73,17 +73,17 @@ \. -- various COPY options: delimiters, oids, NULL string -COPY x (b, c, d, e) from stdin with oids delimiter ',' null 'x'; +COPY x (b, c, d, e) from stdin (oids, delimiter ',', null 'x'); 50,x,45,80,90 51,x,\x,\\x,\\\x 52,x,\,,\\\,,\\ \. -COPY x from stdin WITH DELIMITER AS ';' NULL AS ''; +COPY x from stdin (DELIMITER ';', NULL ''); 3000;;c;; \. -COPY x from stdin WITH DELIMITER AS ':' NULL AS E'\\X'; +COPY x from stdin (DELIMITER ':', NULL E'\\X'); 4000:\X:C:\X:\X 4001:1:empty:: 4002:2:null:\X:\X @@ -108,13 +108,13 @@ INSERT INTO no_oids (a, b) VALUES (20, 30); -- should fail -COPY no_oids FROM stdin WITH OIDS; -COPY no_oids TO stdout WITH OIDS; +COPY no_oids FROM stdin (OIDS); +COPY no_oids TO stdout (OIDS); -- check copy out COPY x TO stdout; COPY x (c, e) TO stdout; -COPY x (b, e) TO stdout WITH NULL 'I''m null'; +COPY x (b, e) TO stdout (NULL 'I''m null'); CREATE TEMP TABLE y ( col1 text, @@ -130,11 +130,23 @@ COPY y TO stdout WITH CSV FORCE QUOTE col2 ESCAPE E'\\'; COPY y TO stdout WITH CSV FORCE QUOTE *; +-- Test new 8.5 syntax + +COPY y TO stdout (CSV); +COPY y TO stdout (CSV, CSV_QUOTE , DELIMITER '|'); +COPY y TO stdout (CSV, CSV_FORCE_QUOTE (col2), CSV_ESCAPE E'\\'); +COPY y TO stdout (CSV, CSV_FORCE_QUOTE *); + +\COPY y TO stdout (CSV) +\COPY y TO stdout (CSV, CSV_QUOTE , DELIMITER '|') +\COPY y TO stdout (CSV, CSV_FORCE_QUOTE (col2), CSV_ESCAPE E'\\') +\COPY y TO stdout (CSV, CSV_FORCE_QUOTE *) + --test that we read consecutive LFs properly CREATE TEMP TABLE testnl (a int, b text, c int); -COPY testnl FROM stdin CSV; +COPY testnl FROM stdin (CSV); 1,"a field with two LFs inside",2 @@ -143,14 +155,14 @@ -- test end of copy marker CREATE TEMP TABLE testeoc (a text); -COPY testeoc FROM stdin CSV; +COPY testeoc FROM stdin (CSV); a\. \.b c\.d "\." \. -COPY testeoc TO stdout CSV; +COPY testeoc TO stdout (CSV); DROP TABLE x, y; DROP FUNCTION fn_x_before(); Index: src/test/regress/sql/aggregates.sql === RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/sql/aggregates.sql,v retrieving revision 1.15 diff -u -r1.15 aggregates.sql --- src/test/regress/sql/aggregates.sql 25 Apr 2009 16:44:56 - 1.15 +++ src/test/regress/sql/aggregates.sql 17 Sep 2009 15:04:06 - @@ -104,7 +104,7 @@ BIT_OR(i4) AS "?" FROM bitwise_test; -COPY bitwise_test FROM STDIN NULL 'null'; +COPY bitwise_test FROM STDIN (NULL 'null'); 1 1 1 1 1 B0101 3 3 3 null2 B0100 7 7 7 3 4 B1100 @@ -171,7 +171,7 @@ BOOL_OR(b3)AS "n" FROM bool_test; -COPY bool_test FROM STDIN NULL 'null'; +COPY bool_test FROM STDIN (NULL 'null'); TRUE nullFALSE null FALSE TRUEnullnull null TRUEFALSE null Index: src/test/regress/sql/copyselect.sql === RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/sql/copyselect.sql,v retrieving revision 1.2 diff -u -r1.2 copyselect.sql --- src/test/regress/sql/copyselect.sql 7 Aug 2008 01:11:52 - 1.2 +++ src/test/regress/sql/copyselect.sql 17 Sep 2009 15:04:06 - @@ -61,7 +61,7 @@ -- -- Test headers, CSV and quotes -- -copy (select t from test1 where id = 1) to stdout csv header force quote t; +copy (select t from test1 where id = 1) to stdout (csv, csv_header, csv_force_quote (t)); -- -- Test psql builtins, plain table -- Index: src/test/regress/expected/aggregates.out === RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/expected/aggregates.out,v retrieving revision 1.19 diff
Re: [HACKERS] generic copy options
Emmanuel Cecchet writes: > Does that mean that we can drop the 7.3 syntax or should we just keep it? I wouldn't object to dropping the 7.3 syntax now, especially if we're about to introduce a new syntax and deprecate the old one ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Tom Lane wrote: Emmanuel Cecchet writes: Tom Lane wrote: psql has MORE need to support old syntax than the backend does, because it's supposed to work against old servers. Well, I wonder how many users just upgrade psql vs upgrade the server. We have established a project policy that psql backslash commands will support servers at least back to 7.4, and a great deal of work has already been expended in support of that goal. It is not within the charter of this patch to ignore or redefine that policy. Does that mean that we can drop the 7.3 syntax or should we just keep it? For future references, where can I find the various project policies? Thanks Emmanuel -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Emmanuel Cecchet writes: > Tom Lane wrote: >> psql has MORE need to support old syntax than the backend does, because >> it's supposed to work against old servers. >> > Well, I wonder how many users just upgrade psql vs upgrade the server. We have established a project policy that psql backslash commands will support servers at least back to 7.4, and a great deal of work has already been expended in support of that goal. It is not within the charter of this patch to ignore or redefine that policy. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
2009/9/17 Emmanuel Cecchet : > Pavel Stehule wrote: >>> >>> Well, I wonder how many users just upgrade psql vs upgrade the server. I >>> was >>> thinking that when users perform a database upgrade their application >>> often >>> remain the same and therefore the server needs to support the old syntax. >>> Unless you are upgrading a machine where a bunch of psql-based scripts >>> are >>> running to update various remote Postgres instances with older versions, >>> I >>> would guess that it is unlikely that someone is going to upgrade psql and >>> keep the old instance of the server on the same machine. >>> I just wonder how many users are using a single psql to manage multiple >>> server instances of different older versions. >>> >> >> What application, that use current copy format for fast data import? I >> thing, so doing incompatible changes of copy statement syntax is very >> bad idea. >> > > The old syntax is still supported in both psql and the server but I am not > sure how many applications are relying on psql to perform a copy operation > (actually a \copy). who knows. \copy is very useful thinks and people who imports data from local use it. I am sure, so this feature is often used, mainly by unix dba. regards Pavel > > manu > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Emmanuel Cecchet wrote: > I just wonder how many users are using a single psql to manage > multiple server instances of different older versions. I do that, but I do try to keep all the active versions on my machine, so that I can use one which exactly matches any of our 100 servers when it matters. (Or I can ssh to the server and use its psql.) -Kevin -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Pavel Stehule wrote: Well, I wonder how many users just upgrade psql vs upgrade the server. I was thinking that when users perform a database upgrade their application often remain the same and therefore the server needs to support the old syntax. Unless you are upgrading a machine where a bunch of psql-based scripts are running to update various remote Postgres instances with older versions, I would guess that it is unlikely that someone is going to upgrade psql and keep the old instance of the server on the same machine. I just wonder how many users are using a single psql to manage multiple server instances of different older versions. What application, that use current copy format for fast data import? I thing, so doing incompatible changes of copy statement syntax is very bad idea. The old syntax is still supported in both psql and the server but I am not sure how many applications are relying on psql to perform a copy operation (actually a \copy). manu -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
2009/9/17 Emmanuel Cecchet : > Tom Lane wrote: >>> >>> While I understand the need for the server to still support the syntax, >>> is it necessary for newer version of psql to support the old syntax? >>> >> >> psql has MORE need to support old syntax than the backend does, because >> it's supposed to work against old servers. >> > > Well, I wonder how many users just upgrade psql vs upgrade the server. I was > thinking that when users perform a database upgrade their application often > remain the same and therefore the server needs to support the old syntax. > Unless you are upgrading a machine where a bunch of psql-based scripts are > running to update various remote Postgres instances with older versions, I > would guess that it is unlikely that someone is going to upgrade psql and > keep the old instance of the server on the same machine. > I just wonder how many users are using a single psql to manage multiple > server instances of different older versions. What application, that use current copy format for fast data import? I thing, so doing incompatible changes of copy statement syntax is very bad idea. regards Pavel Stehule >> >> I wonder though if we couldn't simplify matters. Offhand it seems to me >> that psql doesn't need to validate the command's syntax fully. All it >> really needs to do is find the target filename and replace it with >> STDIN/STDOUT. Could we have it just treat the remainder of the line >> literally, and not worry about the details of what the options might be? >> Let the backend worry about throwing an error if they're bad. >> > > As the only difference between \copy and copy seems to be the ability to > stream the file from the client, I guess that everything else should be sent > as is to the server as you suggest. I'll come with a patch for that today. > > Emmanuel > > -- > Emmanuel Cecchet > Aster Data Systems > Web: http://www.asterdata.com > > > -- > Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Tom Lane wrote: While I understand the need for the server to still support the syntax, is it necessary for newer version of psql to support the old syntax? psql has MORE need to support old syntax than the backend does, because it's supposed to work against old servers. Well, I wonder how many users just upgrade psql vs upgrade the server. I was thinking that when users perform a database upgrade their application often remain the same and therefore the server needs to support the old syntax. Unless you are upgrading a machine where a bunch of psql-based scripts are running to update various remote Postgres instances with older versions, I would guess that it is unlikely that someone is going to upgrade psql and keep the old instance of the server on the same machine. I just wonder how many users are using a single psql to manage multiple server instances of different older versions. I wonder though if we couldn't simplify matters. Offhand it seems to me that psql doesn't need to validate the command's syntax fully. All it really needs to do is find the target filename and replace it with STDIN/STDOUT. Could we have it just treat the remainder of the line literally, and not worry about the details of what the options might be? Let the backend worry about throwing an error if they're bad. As the only difference between \copy and copy seems to be the ability to stream the file from the client, I guess that everything else should be sent as is to the server as you suggest. I'll come with a patch for that today. Emmanuel -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Pavel, I am not sure about syntax change. Isn't better solve this problem well. This is too simple solution. I thinking, so we able to add new parser for COPY statement and share this paraser between SQL and psql. Refactoring COPY to put new parsers seems to be another project. The idea here is that the parser does not have to be changed again if we add new options, we just have to handle them in the COPY code (which will probably have to be refactored at some point as it was discussed in a previous thread). manu 2009/9/17 Emmanuel Cecchet : Robert Haas wrote: I don't think the way the doc changes are formatted is consistent with what we've done elsewhere. I think that breaking the options out as a separate block could be OK (because otherwise they have to be duplicated between COPY TO and COPY FROM) but it should be done more like the way that the SELECT page is done. I looked at the way it is done in SELECT and there is a section per clause (from clause, where clause, ...). So I am not sure how you want to apply that here besides the copy parameters and the option clause. Also, you haven't documented the syntax 100% correctly: the boolean options work just like the boolean explain options - they take an optional argument which if omitted defaults to true, but you can also specify 0, 1, true, false, on, off. See defGetBoolean. So those should be specified as: BINARY [boolean] OIDS [boolean] CSV [boolean] CSV_HEADER [boolean] See how we did it in sql-explain.html. Ok, fixed. I changed the name of the CSV options to prefix them with csv_ to avoid confusion with any future options. I also had to change the grammar to allow '*' as a parameter (needed for cvs_force_quote). You seem to have introduced a LARGE number of unnecessary whitespace changes here which are not going to fly. You need to go through and revert all of those. It's hard to tell what you've really changed here, but also every whitespace change that gets committed is a potential merge conflict for someone else; plus pgindent will eventually change it back, thus creating another potential merge conflict for someone else. Sorry, I overlooked a format in Eclipse that formatted the whole file instead of the block I was working on. This should be fixed now. I am not 100% sold on renaming all of the CSV-specific options to add "csv_". I would like to get an opinion from someone else on whether that is a good idea or not. I am fairly certain it is NOT a good idea to support BOTH the old and new option names, as you've done here. If you're going to rename them, you should update gram.y and change the makeDefElem() calls within the copy_opt_list productions to emit the new names. Agreed for the makeDefElem(). For changing the names, I think that names like 'header', 'escape' and 'quote' are too generic to not conflict with something that is not csv. If you think of another format that could be added to copy, it is likely to re-use the same variable names. The only thing that seems odd is that if you use a CSV_* option, you still have to add CSV [on] to the option list which seems kind of redundant. When we decide to drop the old syntax (in 8.6?), we will be able to clean a lot especially in psql. Considering that we are still carrying syntax that was deprecated in 7.3, I don't think it's likely that we'll phase out the present syntax anywhere nearly that quickly. But it's reasonable to ask whether we should think about removing support for the pre-7.3 syntax altogether for 8.5. It doesn't seem to cost us much to keep that support around, but then again it's been deprecated for seven major releases, so it might be about time. While I understand the need for the server to still support the syntax, is it necessary for newer version of psql to support the old syntax? I am attaching the new version of the patch with the current modifications addressing your comments. Emmanuel -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com ### Eclipse Workspace Patch 1.0 #P Postgres8.5-COPY Index: src/test/regress/sql/copy2.sql === RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/sql/copy2.sql,v retrieving revision 1.18 diff -u -r1.18 copy2.sql --- src/test/regress/sql/copy2.sql 25 Jul 2009 00:07:14 - 1.18 +++ src/test/regress/sql/copy2.sql 17 Sep 2009 03:14:48 - @@ -73,17 +73,17 @@ \. -- various COPY options: delimiters, oids, NULL string -COPY x (b, c, d, e) from stdin with oids delimiter ',' null 'x'; +COPY x (b, c, d, e) from stdin (oids, delimiter ',', null 'x'); 50,x,45,80,90 51,x,\x,\\x,\\\x 52,x,\,,\\\,,\\ \. -COPY x from stdin WITH DELIMITER AS ';' NULL AS ''; +COPY x from stdin (DELIMITER ';', NULL ''); 3000;;c;; \. -COPY x from stdin WITH DELIMITER AS ':' NULL AS E'\\X'; +COPY x from stdin (DELIMITER ':',
Re: [HACKERS] generic copy options
Tom Lane wrote: psql has MORE need to support old syntax than the backend does, because it's supposed to work against old servers. I wonder though if we couldn't simplify matters. Offhand it seems to me that psql doesn't need to validate the command's syntax fully. All it really needs to do is find the target filename and replace it with STDIN/STDOUT. Could we have it just treat the remainder of the line literally, and not worry about the details of what the options might be? Let the backend worry about throwing an error if they're bad. Makes plenty of sense. On a related topic, I'm not sure how we would go about providing psql support for the suggested copy-as-from-target feature that's been discussed recently. That could get mildly ugly. cheers andrew -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Emmanuel Cecchet writes: > Robert Haas wrote: >>> When we decide to drop the old syntax (in 8.6?), we will be able to clean a >>> lot especially in psql. >> Considering that we are still carrying syntax that was deprecated in >> 7.3, I don't think it's likely that we'll phase out the present syntax >> anywhere nearly that quickly. > While I understand the need for the server to still support the syntax, > is it necessary for newer version of psql to support the old syntax? psql has MORE need to support old syntax than the backend does, because it's supposed to work against old servers. I wonder though if we couldn't simplify matters. Offhand it seems to me that psql doesn't need to validate the command's syntax fully. All it really needs to do is find the target filename and replace it with STDIN/STDOUT. Could we have it just treat the remainder of the line literally, and not worry about the details of what the options might be? Let the backend worry about throwing an error if they're bad. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Robert Haas wrote: I don't think the way the doc changes are formatted is consistent with what we've done elsewhere. I think that breaking the options out as a separate block could be OK (because otherwise they have to be duplicated between COPY TO and COPY FROM) but it should be done more like the way that the SELECT page is done. I looked at the way it is done in SELECT and there is a section per clause (from clause, where clause, ...). So I am not sure how you want to apply that here besides the copy parameters and the option clause. Also, you haven't documented the syntax 100% correctly: the boolean options work just like the boolean explain options - they take an optional argument which if omitted defaults to true, but you can also specify 0, 1, true, false, on, off. See defGetBoolean. So those should be specified as: BINARY [boolean] OIDS [boolean] CSV [boolean] CSV_HEADER [boolean] See how we did it in sql-explain.html. Ok, fixed. I changed the name of the CSV options to prefix them with csv_ to avoid confusion with any future options. I also had to change the grammar to allow '*' as a parameter (needed for cvs_force_quote). You seem to have introduced a LARGE number of unnecessary whitespace changes here which are not going to fly. You need to go through and revert all of those. It's hard to tell what you've really changed here, but also every whitespace change that gets committed is a potential merge conflict for someone else; plus pgindent will eventually change it back, thus creating another potential merge conflict for someone else. Sorry, I overlooked a format in Eclipse that formatted the whole file instead of the block I was working on. This should be fixed now. I am not 100% sold on renaming all of the CSV-specific options to add "csv_". I would like to get an opinion from someone else on whether that is a good idea or not. I am fairly certain it is NOT a good idea to support BOTH the old and new option names, as you've done here. If you're going to rename them, you should update gram.y and change the makeDefElem() calls within the copy_opt_list productions to emit the new names. Agreed for the makeDefElem(). For changing the names, I think that names like 'header', 'escape' and 'quote' are too generic to not conflict with something that is not csv. If you think of another format that could be added to copy, it is likely to re-use the same variable names. The only thing that seems odd is that if you use a CSV_* option, you still have to add CSV [on] to the option list which seems kind of redundant. When we decide to drop the old syntax (in 8.6?), we will be able to clean a lot especially in psql. Considering that we are still carrying syntax that was deprecated in 7.3, I don't think it's likely that we'll phase out the present syntax anywhere nearly that quickly. But it's reasonable to ask whether we should think about removing support for the pre-7.3 syntax altogether for 8.5. It doesn't seem to cost us much to keep that support around, but then again it's been deprecated for seven major releases, so it might be about time. While I understand the need for the server to still support the syntax, is it necessary for newer version of psql to support the old syntax? I am attaching the new version of the patch with the current modifications addressing your comments. Emmanuel -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com ### Eclipse Workspace Patch 1.0 #P Postgres8.5-COPY Index: src/test/regress/sql/copy2.sql === RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/sql/copy2.sql,v retrieving revision 1.18 diff -u -r1.18 copy2.sql --- src/test/regress/sql/copy2.sql 25 Jul 2009 00:07:14 - 1.18 +++ src/test/regress/sql/copy2.sql 17 Sep 2009 03:14:48 - @@ -73,17 +73,17 @@ \. -- various COPY options: delimiters, oids, NULL string -COPY x (b, c, d, e) from stdin with oids delimiter ',' null 'x'; +COPY x (b, c, d, e) from stdin (oids, delimiter ',', null 'x'); 50,x,45,80,90 51,x,\x,\\x,\\\x 52,x,\,,\\\,,\\ \. -COPY x from stdin WITH DELIMITER AS ';' NULL AS ''; +COPY x from stdin (DELIMITER ';', NULL ''); 3000;;c;; \. -COPY x from stdin WITH DELIMITER AS ':' NULL AS E'\\X'; +COPY x from stdin (DELIMITER ':', NULL E'\\X'); 4000:\X:C:\X:\X 4001:1:empty:: 4002:2:null:\X:\X @@ -108,13 +108,13 @@ INSERT INTO no_oids (a, b) VALUES (20, 30); -- should fail -COPY no_oids FROM stdin WITH OIDS; -COPY no_oids TO stdout WITH OIDS; +COPY no_oids FROM stdin (OIDS); +COPY no_oids TO stdout (OIDS); -- check copy out COPY x TO stdout; COPY x (c, e) TO stdout; -COPY x (b, e) TO stdout WITH NULL 'I''m null'; +COPY x (b, e) TO stdout (NULL 'I''m null'); CREATE TEMP TABLE y ( col1 text, @@ -130,11 +130,23 @@ COPY y TO stdout WITH CSV FORCE QUOTE col2 ESCAPE E'\\'; COPY y TO stdout WITH CSV FORCE QUOTE
Re: [HACKERS] generic copy options
On Wed, Sep 16, 2009 at 6:43 PM, Emmanuel Cecchet wrote: > Here is a new version of the patch with an updated doc and psql. Thanks, that's great. I don't think the way the doc changes are formatted is consistent with what we've done elsewhere. I think that breaking the options out as a separate block could be OK (because otherwise they have to be duplicated between COPY TO and COPY FROM) but it should be done more like the way that the SELECT page is done. Also, you haven't documented the syntax 100% correctly: the boolean options work just like the boolean explain options - they take an optional argument which if omitted defaults to true, but you can also specify 0, 1, true, false, on, off. See defGetBoolean. So those should be specified as: BINARY [boolean] OIDS [boolean] CSV [boolean] CSV_HEADER [boolean] See how we did it in sql-explain.html. > I changed the name of the CSV options to prefix them with csv_ to avoid > confusion with any future options. I also had to change the grammar to allow > '*' as a parameter (needed for cvs_force_quote). You seem to have introduced a LARGE number of unnecessary whitespace changes here which are not going to fly. You need to go through and revert all of those. It's hard to tell what you've really changed here, but also every whitespace change that gets committed is a potential merge conflict for someone else; plus pgindent will eventually change it back, thus creating another potential merge conflict for someone else. I am not 100% sold on renaming all of the CSV-specific options to add "csv_". I would like to get an opinion from someone else on whether that is a good idea or not. I am fairly certain it is NOT a good idea to support BOTH the old and new option names, as you've done here. If you're going to rename them, you should update gram.y and change the makeDefElem() calls within the copy_opt_list productions to emit the new names. > When we decide to drop the old syntax (in 8.6?), we will be able to clean a > lot especially in psql. Considering that we are still carrying syntax that was deprecated in 7.3, I don't think it's likely that we'll phase out the present syntax anywhere nearly that quickly. But it's reasonable to ask whether we should think about removing support for the pre-7.3 syntax altogether for 8.5. It doesn't seem to cost us much to keep that support around, but then again it's been deprecated for seven major releases, so it might be about time. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Robert, Here is a new version of the patch with an updated doc and psql. I changed the name of the CSV options to prefix them with csv_ to avoid confusion with any future options. I also had to change the grammar to allow '*' as a parameter (needed for cvs_force_quote). When we decide to drop the old syntax (in 8.6?), we will be able to clean a lot especially in psql. Emmanuel On Mon, Sep 14, 2009 at 2:51 PM, Emmanuel Cecchet wrote: This looks good. Shoud I try to elaborate on that for the patch with error logging and autopartitioning in COPY? That make sense to me. You shouldn't need to do anything else in gram.y; whatever you want to add should just involve changing copy.c. If not, please post the details. We also need to fix the psql end of this, and the docs... any interest in taking a crack at either of those? ...Robert -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com ### Eclipse Workspace Patch 1.0 #P Postgres8.5-COPY Index: src/test/regress/sql/copy2.sql === RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/sql/copy2.sql,v retrieving revision 1.18 diff -u -r1.18 copy2.sql --- src/test/regress/sql/copy2.sql 25 Jul 2009 00:07:14 - 1.18 +++ src/test/regress/sql/copy2.sql 16 Sep 2009 22:37:31 - @@ -73,17 +73,17 @@ \. -- various COPY options: delimiters, oids, NULL string -COPY x (b, c, d, e) from stdin with oids delimiter ',' null 'x'; +COPY x (b, c, d, e) from stdin (oids, delimiter ',', null 'x'); 50,x,45,80,90 51,x,\x,\\x,\\\x 52,x,\,,\\\,,\\ \. -COPY x from stdin WITH DELIMITER AS ';' NULL AS ''; +COPY x from stdin (DELIMITER ';', NULL ''); 3000;;c;; \. -COPY x from stdin WITH DELIMITER AS ':' NULL AS E'\\X'; +COPY x from stdin (DELIMITER ':', NULL E'\\X'); 4000:\X:C:\X:\X 4001:1:empty:: 4002:2:null:\X:\X @@ -108,13 +108,13 @@ INSERT INTO no_oids (a, b) VALUES (20, 30); -- should fail -COPY no_oids FROM stdin WITH OIDS; -COPY no_oids TO stdout WITH OIDS; +COPY no_oids FROM stdin (OIDS); +COPY no_oids TO stdout (OIDS); -- check copy out COPY x TO stdout; COPY x (c, e) TO stdout; -COPY x (b, e) TO stdout WITH NULL 'I''m null'; +COPY x (b, e) TO stdout (NULL 'I''m null'); CREATE TEMP TABLE y ( col1 text, @@ -130,11 +130,23 @@ COPY y TO stdout WITH CSV FORCE QUOTE col2 ESCAPE E'\\'; COPY y TO stdout WITH CSV FORCE QUOTE *; +-- Test new 8.5 syntax + +COPY y TO stdout (CSV); +COPY y TO stdout (CSV, CSV_QUOTE , DELIMITER '|'); +COPY y TO stdout (CSV, CSV_FORCE_QUOTE (col2), CSV_ESCAPE E'\\'); +COPY y TO stdout (CSV, CSV_FORCE_QUOTE *); + +\COPY y TO stdout (CSV) +\COPY y TO stdout (CSV, CSV_QUOTE , DELIMITER '|') +\COPY y TO stdout (CSV, CSV_FORCE_QUOTE (col2), CSV_ESCAPE E'\\') +\COPY y TO stdout (CSV, CSV_FORCE_QUOTE *) + --test that we read consecutive LFs properly CREATE TEMP TABLE testnl (a int, b text, c int); -COPY testnl FROM stdin CSV; +COPY testnl FROM stdin (CSV); 1,"a field with two LFs inside",2 @@ -143,14 +155,14 @@ -- test end of copy marker CREATE TEMP TABLE testeoc (a text); -COPY testeoc FROM stdin CSV; +COPY testeoc FROM stdin (CSV); a\. \.b c\.d "\." \. -COPY testeoc TO stdout CSV; +COPY testeoc TO stdout (CSV); DROP TABLE x, y; DROP FUNCTION fn_x_before(); Index: src/test/regress/sql/aggregates.sql === RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/sql/aggregates.sql,v retrieving revision 1.15 diff -u -r1.15 aggregates.sql --- src/test/regress/sql/aggregates.sql 25 Apr 2009 16:44:56 - 1.15 +++ src/test/regress/sql/aggregates.sql 16 Sep 2009 22:37:31 - @@ -104,7 +104,7 @@ BIT_OR(i4) AS "?" FROM bitwise_test; -COPY bitwise_test FROM STDIN NULL 'null'; +COPY bitwise_test FROM STDIN (NULL 'null'); 1 1 1 1 1 B0101 3 3 3 null2 B0100 7 7 7 3 4 B1100 @@ -171,7 +171,7 @@ BOOL_OR(b3)AS "n" FROM bool_test; -COPY bool_test FROM STDIN NULL 'null'; +COPY bool_test FROM STDIN (NULL 'null'); TRUE nullFALSE null FALSE TRUEnullnull null TRUEFALSE null Index: src/test/regress/sql/copyselect.sql === RCS file: /home/manu/cvsrepo/pgsql/src/test/regress/sql/copyselect.sql,v retrieving revision 1.2 diff -u -r1.2 copyselect.sql --- src/test/regress/sql/copyselect.sql 7 Aug 2008 01:11:52 - 1.2 +++ src/test/regress/sql/copyselect.sql 16 Sep 2009 22:37:31 - @@ -61,7 +61,7 @@ -- -- Test headers, CSV and quotes -- -copy (select t from test1 where id = 1) to stdout csv header force quote t; +copy (select t from test1 where id = 1) to stdout (csv, csv_header, csv_force_quote (t)); -- -- Test psql builtins, plain table -- Index: src/test/regress/expected/aggregates.out =
Re: [HACKERS] generic copy options
Robert Haas writes: > On Mon, Sep 14, 2009 at 3:25 PM, Emmanuel Cecchet wrote: >> I have never looked at the psql code but that could be a good way to get >> started on that. If you can point me at where to look at, I'll give it a >> try. > I don't know either off the top of my head, but I'll go look for it > when I get a chance. src/bin/psql/copy.c ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
On Mon, Sep 14, 2009 at 3:25 PM, Emmanuel Cecchet wrote: > Robert Haas wrote: >> >> On Mon, Sep 14, 2009 at 2:51 PM, Emmanuel Cecchet >> wrote: >> >>> >>> This looks good. Shoud I try to elaborate on that for the patch with >>> error >>> logging and autopartitioning in COPY? >>> >> >> That make sense to me. You shouldn't need to do anything else in >> gram.y; whatever you want to add should just involve changing copy.c. >> If not, please post the details. > > Ok, I'll keep you posted. >> >> We also need to fix the psql end of this, and the docs... any >> interest in taking a crack at either of those? > > I can certainly help with the doc. If you have the time to revise the docs to describe this new syntax, that would be great. > I have never looked at the psql code but that could be a good way to get > started on that. If you can point me at where to look at, I'll give it a > try. I don't know either off the top of my head, but I'll go look for it when I get a chance. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
Robert Haas wrote: On Mon, Sep 14, 2009 at 2:51 PM, Emmanuel Cecchet wrote: This looks good. Shoud I try to elaborate on that for the patch with error logging and autopartitioning in COPY? That make sense to me. You shouldn't need to do anything else in gram.y; whatever you want to add should just involve changing copy.c. If not, please post the details. Ok, I'll keep you posted. We also need to fix the psql end of this, and the docs... any interest in taking a crack at either of those? I can certainly help with the doc. I have never looked at the psql code but that could be a good way to get started on that. If you can point me at where to look at, I'll give it a try. Emmanuel -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
On Mon, Sep 14, 2009 at 2:51 PM, Emmanuel Cecchet wrote: > This looks good. Shoud I try to elaborate on that for the patch with error > logging and autopartitioning in COPY? That make sense to me. You shouldn't need to do anything else in gram.y; whatever you want to add should just involve changing copy.c. If not, please post the details. We also need to fix the psql end of this, and the docs... any interest in taking a crack at either of those? ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] generic copy options
This looks good. Shoud I try to elaborate on that for the patch with error logging and autopartitioning in COPY? manu Robert Haas wrote: Here's a half-baked proof of concept for the above approach. This probably needs more testing than I've given it, and I haven't attempted to fix the psql parser or update the documentation, but it's at least an outline of a solution. I did patch all the regression tests to use the new syntax, so you can look at that part of the patch to get a flavor for it. If this is broadly acceptable I can attempt to nail down the details, or someone else is welcome to pick it up. It's on my git repo as well, as usual. -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] generic copy options
On Fri, Sep 11, 2009 at 5:45 PM, Robert Haas wrote: > On Fri, Sep 11, 2009 at 5:32 PM, Tom Lane wrote: >> Robert Haas writes: >>> The biggest problem I have with this change is that it's going to >>> massively break anyone who is using the existing COPY syntax. >> >> Why? We'd certainly still support the old syntax for existing options, >> just as we did with EXPLAIN. > > None of the syntax proposals upthread had that property, which doesn't > mean we can't do it. However, we'd need some way to differentiate the > old syntax from the new one. I guess we could throw an unnecessary set > of parentheses around the option list (blech), but you have to be able > to tell from the first token which kind of list you're reading if you > want to support both sets of syntax. Here's a half-baked proof of concept for the above approach. This probably needs more testing than I've given it, and I haven't attempted to fix the psql parser or update the documentation, but it's at least an outline of a solution. I did patch all the regression tests to use the new syntax, so you can look at that part of the patch to get a flavor for it. If this is broadly acceptable I can attempt to nail down the details, or someone else is welcome to pick it up. It's on my git repo as well, as usual. ...Robert *** a/src/backend/commands/copy.c --- b/src/backend/commands/copy.c *** *** 25,30 --- 25,31 #include "catalog/namespace.h" #include "catalog/pg_type.h" #include "commands/copy.h" + #include "commands/defrem.h" #include "commands/trigger.h" #include "executor/executor.h" #include "libpq/libpq.h" *** *** 745,751 DoCopy(const CopyStmt *stmt, const char *queryString) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"))); ! cstate->binary = intVal(defel->arg); } else if (strcmp(defel->defname, "oids") == 0) { --- 746,752 ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"))); ! cstate->binary = defGetBoolean(defel); } else if (strcmp(defel->defname, "oids") == 0) { *** *** 753,759 DoCopy(const CopyStmt *stmt, const char *queryString) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"))); ! cstate->oids = intVal(defel->arg); } else if (strcmp(defel->defname, "delimiter") == 0) { --- 754,760 ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"))); ! cstate->oids = defGetBoolean(defel); } else if (strcmp(defel->defname, "delimiter") == 0) { *** *** 761,767 DoCopy(const CopyStmt *stmt, const char *queryString) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"))); ! cstate->delim = strVal(defel->arg); } else if (strcmp(defel->defname, "null") == 0) { --- 762,768 ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"))); ! cstate->delim = defGetString(defel); } else if (strcmp(defel->defname, "null") == 0) { *** *** 769,775 DoCopy(const CopyStmt *stmt, const char *queryString) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"))); ! cstate->null_print = strVal(defel->arg); } else if (strcmp(defel->defname, "csv") == 0) { --- 770,776 ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"))); ! cstate->null_print = defGetString(defel); } else if (strcmp(defel->defname, "csv") == 0) { *** *** 777,783 DoCopy(const CopyStmt *stmt, const char *queryString) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"))); ! cstate->csv_mode = intVal(defel->arg); } else if (strcmp(defel->defname, "header") == 0) { --- 778,784 ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"))); ! cstate->csv_mode = defGetBoolean(defel); } else if (strcmp(defel->defname, "header") == 0) { *** *** 785,791 DoCopy(const CopyStmt *stmt, const char *queryString) ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"))); ! cstate->header_line = intVal(defel->arg); } else if (strcmp(defel->defname, "quote") == 0) { --- 786,792 ereport(ERROR, (errcode(ERRCODE_SYNTAX_ERROR), errmsg("conflicting or redundant options"))); ! cstate->header_line = defGetBoolean(defel); } else if (strcmp(defel->defname, "quote") == 0) { *** *** 793,799 DoCopy(const CopyStmt *stm