Re: [HACKERS] [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet

[David G. Johnston]

On Tue, May 31, 2016 at 3:55 PM, Nikolay Shaplov 
wrote:

> В письме от 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.
​

Re: [HACKERS] [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet

[Nikolay Shaplov]

В письме от 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...


-- 
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] [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet

[Robert Haas]

On Tue, May 31, 2016 at 12:34 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Thu, May 26, 2016 at 3:28 PM, Tom Lane  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


-- 
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] [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet

[Tom Lane]

Robert Haas  writes:
> On Thu, May 26, 2016 at 3:28 PM, Tom Lane  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


-- 
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] [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet

[Robert Haas]

On Thu, May 26, 2016 at 3:28 PM, Tom Lane  wrote:
> Nikolay Shaplov  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


-- 
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] [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet

[Tom Lane]

Nikolay Shaplov  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


-- 
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] [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet

[Nikolay Shaplov]

В письме от 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


-- 
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] [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet

[Tom Lane]

Nikolay Shaplov  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


-- 
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] [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet

[David G. Johnston]

On Thu, May 26, 2016 at 8:09 AM, Nikolay Shaplov 
wrote:

> В письме от 24 мая 2016 12:47:20 пользователь Tom Lane написал:
> > Nikolay Shaplov  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: [HACKERS] [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet

[Nikolay Shaplov]

В письме от 24 мая 2016 12:47:20 пользователь Tom Lane написал:
> Nikolay Shaplov  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


-- 
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] [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet

[Tom Lane]

Nikolay Shaplov  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


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] [PATCH][Documination] Add optional USING keyword before opclass name in INSERT statemet

[Nikolay Shaplov]

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 Companydiff --git a/doc/src/sgml/ref/create_index.sgml b/doc/src/sgml/ref/create_index.sgml
index 7dee405..788bc3f 100644
--- a/doc/src/sgml/ref/create_index.sgml
+++ b/doc/src/sgml/ref/create_index.sgml
@@ -22,7 +22,7 @@ PostgreSQL documentation
  
 
 CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ [ IF NOT EXISTS ] name ] ON table_name [ USING method ]
-( { column_name | ( expression ) } [ COLLATE collation ] [ opclass ] [ ASC | DESC ] [ NULLS { FIRST | LAST } ] [, ...] )
+( { column_name | ( expression ) } [ COLLATE collation ] [ [ USING ] opclass ] [ ASC | DESC ] [ NULLS { FIRST | LAST } ] [, ...] )
 [ WITH ( storage_parameter = value [, ... ] ) ]
 [ TABLESPACE tablespace_name ]
 [ WHERE predicate ]

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers