Thread: [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet
[PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet
From
Nikolay Shaplov
Date:
If I read gram.y code for insert statement, I see that there is an optional
USING keyword before opclass name
opt_class: any_name { $$ = $1; }
| USING any_name { $$ = $2; }
| /*EMPTY*/ { $$ = NIL; }
;
but it the documentation this keyword is omitted.
I'd like to offer a patch that fixes this problem
--
Nikolay Shaplov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Attachment
Re: [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet
From
Tom Lane
Date:
Nikolay Shaplov <n.shaplov@postgrespro.ru> writes:
> If I read gram.y code for insert statement, I see that there is an optional
> USING keyword before opclass name
> opt_class: any_name { $$ = $1; }
> | USING any_name { $$ = $2; }
> | /*EMPTY*/ { $$ = NIL; }
> ;
> but it the documentation this keyword is omitted.
I think we should seriously consider fixing this code/docs discrepancy
by making the code match the docs, not vice versa. That is, let's just
remove the USING alternative here entirely.
If we wanted to make the docs match the code, it would not only be
CREATE INDEX that would have to be patched, because that's not the
only place that index_elem can appear. As far as I can find in a
quick search, none of the relevant statements have ever documented
that USING is allowed here; nor does it appear that any client-side
code of ours makes use of the keyword.
Also, because USING is already used elsewhere in CREATE INDEX (to
introduce the optional index AM name), I think that documenting its
use in this clause would add confusion not subtract it. References
to "the USING clause in CREATE INDEX" would become ambiguous.
This wouldn't be something to back-patch, of course, but I think it's
an entirely reasonable change to make in HEAD.
Comments?
regards, tom lane
Re: [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet
From
Nikolay Shaplov
Date:
В письме от 24 мая 2016 12:47:20 пользователь Tom Lane написал:
> Nikolay Shaplov <n.shaplov@postgrespro.ru> writes:
> > If I read gram.y code for insert statement, I see that there is an
> > optional
> > USING keyword before opclass name
> >
> > opt_class: any_name { $$ = $1; }
> >
> > | USING any_name { $$ = $2; }
> > | /*EMPTY*/ { $$ = NIL; }
> >
> > ;
> >
> > but it the documentation this keyword is omitted.
>
> I think we should seriously consider fixing this code/docs discrepancy
> by making the code match the docs, not vice versa. That is, let's just
> remove the USING alternative here entirely.
>
> If we wanted to make the docs match the code, it would not only be
> CREATE INDEX that would have to be patched, because that's not the
> only place that index_elem can appear. As far as I can find in a
> quick search, none of the relevant statements have ever documented
> that USING is allowed here; nor does it appear that any client-side
> code of ours makes use of the keyword.
>
> Also, because USING is already used elsewhere in CREATE INDEX (to
> introduce the optional index AM name), I think that documenting its
> use in this clause would add confusion not subtract it. References
> to "the USING clause in CREATE INDEX" would become ambiguous.
>
> This wouldn't be something to back-patch, of course, but I think it's
> an entirely reasonable change to make in HEAD.
>
> Comments?
I have two arguments for not removing USING there.
1. Backward compatibility. Are you sure, that nobody ever wrote a code using
this "USING" keyword? I would say it is unlikely, but will not be sure 100%.
Dropping this backward compatibility (even with small chance that it was ever
used) for no practical reason is not wise at all. If it might bring some pain
to somebody, then why do it after all...
2. I think expression with USING in it is more human readable:
CREATE INDEX (xxx op_yyy);
is less sensible then
CREATE INDEX (xxx USING op_yyy);
in my opinion. In second example person that does not know SQL at all, will
understand that xxx is main object or action, and op_yyy is about how this xxx
will be done or used.
If somebody would like to write more readable code, why we should forbid it to
him?
2.1. As far as I can get the general idea of SQL, there is a tendency to put
keywords (optional or not) between to object names. Like this
SELECT a _AS_ b from ....
I like this tendency
2.2. I am not familiar with SQL ISO standart, and I suppose there is no USING
at all in that case, but I think it would be good to look there to check for
it or for something similar
2.3. And the last, when I found out about this keyword, I started to use it in
my SQL statements that I use while development, and I just liked it. I will
miss it if you remove it ;-)
--
Nikolay Shaplov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company
Re: [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet
From
"David G. Johnston"
Date:
В письме от 24 мая 2016 12:47:20 пользователь Tom Lane написал:
> Nikolay Shaplov <n.shaplov@postgrespro.ru> writes:
> > If I read gram.y code for insert statement, I see that there is an
> > optional
> > USING keyword before opclass name
> >
> > opt_class: any_name { $$ = $1; }
> >
> > | USING any_name { $$ = $2; }
> > | /*EMPTY*/ { $$ = NIL; }
> >
> > ;
> >
> > but it the documentation this keyword is omitted.
>
> I think we should seriously consider fixing this code/docs discrepancy
> by making the code match the docs, not vice versa. That is, let's just
> remove the USING alternative here entirely.
>
> If we wanted to make the docs match the code, it would not only be
> CREATE INDEX that would have to be patched, because that's not the
> only place that index_elem can appear. As far as I can find in a
> quick search, none of the relevant statements have ever documented
> that USING is allowed here; nor does it appear that any client-side
> code of ours makes use of the keyword.
>
> Also, because USING is already used elsewhere in CREATE INDEX (to
> introduce the optional index AM name), I think that documenting its
> use in this clause would add confusion not subtract it. References
> to "the USING clause in CREATE INDEX" would become ambiguous.
>
> This wouldn't be something to back-patch, of course, but I think it's
> an entirely reasonable change to make in HEAD.
>
> Comments?
I have two arguments for not removing USING there.
1. Backward compatibility. Are you sure, that nobody ever wrote a code using
this "USING" keyword? I would say it is unlikely, but will not be sure 100%.
Dropping this backward compatibility (even with small chance that it was ever
used) for no practical reason is not wise at all. If it might bring some pain
to somebody, then why do it after all...
IIUC, since pg_dump/pg_restore never includes this there is not hazard on upgrading or restoration. Furthermore, CREATE INDEX is an administrative function and thus not likely to cause applications to become inoperative.
The surface area here is small enough that the decision to disallow should not be taken off the table.
2. I think expression with USING in it is more human readable:
CREATE INDEX (xxx op_yyy);
is less sensible then
CREATE INDEX (xxx USING op_yyy);
in my opinion. In second example person that does not know SQL at all, will
understand that xxx is main object or action, and op_yyy is about how this xxx
will be done or used.
If somebody would like to write more readable code, why we should forbid it to
him?
I agree.
The argument that having a second portion of the query utilizing the USING keyword would make explanation and comprehension more difficult is also one I agree with.
That said we apparently used to interject the word WITH in between but that ended up generating a potential ambiguity so WITH was changed to USING. This was circa 1997...
The authors of the docs for the past nearly 20 years have assumed that there is no USING clause in that location.
If someone was willing to write a doc patch that passed muster before 10.0 goes live its possible that we'd revert the change and commit the doc patch. The cost/benefit of that effort is not particularly appealing and the project seems content to take the more expedient (and now without its own merits) path forward.
2.1. As far as I can get the general idea of SQL, there is a tendency to put
keywords (optional or not) between to object names. Like this
SELECT a _AS_ b from ....
I like this tendency
Not germane to this discussion.
2.2. I am not familiar with SQL ISO standart, and I suppose there is no USING
at all in that case, but I think it would be good to look there to check for
it or for something similar
Indexes are outside the purview of the ISO SQL Standard.
2.3. And the last, when I found out about this keyword, I started to use it in
my SQL statements that I use while development, and I just liked it. I will
miss it if you remove it ;-)
Thank you for your patronage and your sacrifice.
Is there an address where we can send your purple heart :)
While not a great policy to adhere to, a reversion in a 10.x patch release wouldn't be particularly difficult should people choose to complain rather than adapt.
David J.
Re: [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet
From
Tom Lane
Date:
Nikolay Shaplov <n.shaplov@postgrespro.ru> writes:
> В письме от 24 мая 2016 12:47:20 пользователь Tom Lane написал:
>> I think we should seriously consider fixing this code/docs discrepancy
>> by making the code match the docs, not vice versa. That is, let's just
>> remove the USING alternative here entirely.
> I have two arguments for not removing USING there.
> 1. Backward compatibility. Are you sure, that nobody ever wrote a code using
> this "USING" keyword? I would say it is unlikely, but will not be sure 100%.
I would say the same. However, we make incompatible changes in every
major release, and many of them are way harder to deal with than this.
> 2. I think expression with USING in it is more human readable:
> CREATE INDEX (xxx op_yyy);
> is less sensible then
> CREATE INDEX (xxx USING op_yyy);
Yes. If we were working in a green field, it would have been better to
make USING (or some other reserved word) required, not optional, there.
That would have been better for readability and would have avoided some
syntactic headaches, such as the need to parenthesize the expressions in
expression indexes. However, we're about 18 years too late to make that
decision. Opclass with no introductory keyword is the entrenched standard
at this point, and we're never going to be able to remove it.
regards, tom lane
Re: [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet
From
Nikolay Shaplov
Date:
В письме от 26 мая 2016 10:05:56 пользователь Tom Lane написал: > > 2. I think expression with USING in it is more human readable: > > CREATE INDEX (xxx op_yyy); > > is less sensible then > > CREATE INDEX (xxx USING op_yyy); > > Yes. If we were working in a green field, it would have been better to > make USING (or some other reserved word) required, not optional, there. > That would have been better for readability and would have avoided some > syntactic headaches, such as the need to parenthesize the expressions in > expression indexes. However, we're about 18 years too late to make that > decision. Opclass with no introductory keyword is the entrenched standard > at this point, and we're never going to be able to remove it. No, but we cat keep "USING" as an optional keyword there as it were, just mention it in the docs. It seems logical to me. // Actually I did not expected any discussion for this case. Documentations missed an optional keyword, documentation should be fixed. I am surprised that there can be any other opinions ;-) -- Nikolay Shaplov Postgres Professional: http://www.postgrespro.com Russian Postgres Company
Re: [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet
From
Tom Lane
Date:
Nikolay Shaplov <n.shaplov@postgrespro.ru> writes:
> Actually I did not expected any discussion for this case. Documentations
> missed an optional keyword, documentation should be fixed.
99% of the time, you'd be right. But this is an unusual case, for the
reasons I mentioned before.
regards, tom lane
Re: [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet
From
Robert Haas
Date:
On Thu, May 26, 2016 at 3:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Nikolay Shaplov <n.shaplov@postgrespro.ru> writes: >> Actually I did not expected any discussion for this case. Documentations >> missed an optional keyword, documentation should be fixed. > > 99% of the time, you'd be right. But this is an unusual case, for the > reasons I mentioned before. I tend to agree with Nikolay. I can't see much upside in making this change. At best, nothing will break. At worst, something will break. But how do we actually come out ahead? The only thing you've offered is that it might make it easier to make the relevant documentation pages 100% clear, but I feel like a man of your elocution can probably surmount that impediment. I guess we might save a few parser states, too. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet
From
Tom Lane
Date:
Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, May 26, 2016 at 3:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> 99% of the time, you'd be right. But this is an unusual case, for the
>> reasons I mentioned before.
> I tend to agree with Nikolay. I can't see much upside in making this
> change. At best, nothing will break. At worst, something will break.
> But how do we actually come out ahead?
We come out ahead by not having to make the documentation more confusing.
Basically, we have the opportunity to fix an ancient mistake here at
very low cost. I do not think that doubling down on the mistake is
a better answer.
regards, tom lane
Re: [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet
From
Robert Haas
Date:
On Tue, May 31, 2016 at 12:34 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Robert Haas <robertmhaas@gmail.com> writes: >> On Thu, May 26, 2016 at 3:28 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote: >>> 99% of the time, you'd be right. But this is an unusual case, for the >>> reasons I mentioned before. > >> I tend to agree with Nikolay. I can't see much upside in making this >> change. At best, nothing will break. At worst, something will break. >> But how do we actually come out ahead? > > We come out ahead by not having to make the documentation more confusing. > > Basically, we have the opportunity to fix an ancient mistake here at > very low cost. I do not think that doubling down on the mistake is > a better answer. I'm not convinced, but we don't have to agree on everything... -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet
From
Nikolay Shaplov
Date:
В письме от 31 мая 2016 15:38:38 пользователь Robert Haas написал: > >>> 99% of the time, you'd be right. But this is an unusual case, for the > >>> reasons I mentioned before. > >> > >> I tend to agree with Nikolay. I can't see much upside in making this > >> change. At best, nothing will break. At worst, something will break. > >> But how do we actually come out ahead? > > > > We come out ahead by not having to make the documentation more confusing. > > > > Basically, we have the opportunity to fix an ancient mistake here at > > very low cost. I do not think that doubling down on the mistake is > > a better answer. > > I'm not convinced, but we don't have to agree on everything... I am not convinced too. But I will not argue hard for the patch as far as my main goal was to report inconsistency. Through the I consider Tom's proposal quite strange...
Re: [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet
From
"David G. Johnston"
Date:
В письме от 31 мая 2016 15:38:38 пользователь Robert Haas написал:
> >>> 99% of the time, you'd be right. But this is an unusual case, for the
> >>> reasons I mentioned before.
> >>
> >> I tend to agree with Nikolay. I can't see much upside in making this
> >> change. At best, nothing will break. At worst, something will break.
> >> But how do we actually come out ahead?
> >
> > We come out ahead by not having to make the documentation more confusing.
> >
> > Basically, we have the opportunity to fix an ancient mistake here at
> > very low cost. I do not think that doubling down on the mistake is
> > a better answer.
>
> I'm not convinced, but we don't have to agree on everything...
I am not convinced too. But I will not argue hard for the patch as far as my
main goal was to report inconsistency. Through the I consider Tom's proposal
quite strange...
We've recently chosen to not document the "ANALYZE -> ANALYSE" syntax, and I'm sure there are other examples, so I don't see why the status quo (pre-Tom's patch) is unacceptable...if adding USING to the synopsis is prone to cause confusion then don't; but lets not break existing uses that in no way harm the project.
Otherwise I presume Tom is correct that the true fix is more than a single word in one file of our documentation. If you want to see it stay and be documented there needs to be a complete proposal that at least gets, even if grudging, approval from a couple of people and a committer.
David J.