Patching documentation of ALTER TABLE re column type changes on binary-coercible fields

Started by Mike Lissnerover 6 years ago5 messageshackers
Jump to latest
#1Mike Lissner
mlissner@michaeljaylissner.com

Hi, first patch here and first post to pgsql-hackers. Here goes.

Enclosed please find a patch to tweak the documentation of the ALTER TABLE
page. I believe this patch is ready to be applied to master and backported
all the way to 9.2.

On the ALTER TABLE page, it currently notes that if you change the type of
a column, even to a binary coercible type:

any indexes on the affected columns must still be rebuilt.

It appears this hasn't been true for about eight years, since 367bc426a.

Here's the discussion of the topic from earlier today and yesterday:

/messages/by-id/CAMp9=ExXtH0NeF+LTsNrew_oXycAJTNVKbRYnqgoEAT01t=67A@mail.gmail.com

I haven't run tests, but I presume they'll be unaffected by a documentation
change.

I've made an effort to follow the example of other people's patches I
looked at, but I haven't contributed here before. Happy to take another
stab at this if this doesn't hit the mark — though I hope it does. I love
and appreciate Postgresql and hope that I can do my little part to make it
better.

For the moment, I haven't added this to commitfest. I don't know what it
is, but I suspect this is small enough somebody will just pick it up.

Mike

Attachments:

Documentation__Binary_coercible_types_don't_do_index_rewrite.patchtext/x-patch; charset=US-ASCII; name=Documentation__Binary_coercible_types_don't_do_index_rewrite.patchDownload+3-4
#2Mike Lissner
mlissner@michaeljaylissner.com
In reply to: Mike Lissner (#1)
[Patch]: Documentation of ALTER TABLE re column type changes on binary-coercible fields

Hi all, I didn't get any replies to this. Is this the right way to send in
a patch to the docs?

Thanks,

Mike

On Thu, Jan 23, 2020 at 11:01 PM Mike Lissner <
mlissner@michaeljaylissner.com> wrote:

Show quoted text

Hi, first patch here and first post to pgsql-hackers. Here goes.

Enclosed please find a patch to tweak the documentation of the ALTER TABLE
page. I believe this patch is ready to be applied to master and backported
all the way to 9.2.

On the ALTER TABLE page, it currently notes that if you change the type of
a column, even to a binary coercible type:

any indexes on the affected columns must still be rebuilt.

It appears this hasn't been true for about eight years, since 367bc426a.

Here's the discussion of the topic from earlier today and yesterday:

/messages/by-id/CAMp9=ExXtH0NeF+LTsNrew_oXycAJTNVKbRYnqgoEAT01t=67A@mail.gmail.com

I haven't run tests, but I presume they'll be unaffected by a
documentation change.

I've made an effort to follow the example of other people's patches I
looked at, but I haven't contributed here before. Happy to take another
stab at this if this doesn't hit the mark — though I hope it does. I love
and appreciate Postgresql and hope that I can do my little part to make it
better.

For the moment, I haven't added this to commitfest. I don't know what it
is, but I suspect this is small enough somebody will just pick it up.

Mike

Attachments:

Documentation__Binary_coercible_types_don't_do_index_rewrite.patchtext/x-patch; charset=US-ASCII; name=Documentation__Binary_coercible_types_don't_do_index_rewrite.patchDownload+3-4
#3Mark Dilger
mark.dilger@enterprisedb.com
In reply to: Mike Lissner (#2)
Re: [Patch]: Documentation of ALTER TABLE re column type changes on binary-coercible fields

On Jan 28, 2020, at 10:55 AM, Mike Lissner <mlissner@michaeljaylissner.com> wrote:

Hi all, I didn't get any replies to this. Is this the right way to send in a patch to the docs?

Yes, your patch has been received, thanks. I don’t know if anybody is reviewing it, but typically you don’t hear back on a patch until somebody has reviewed it and has an opinion to share with you.

The feeling, “Hey, I submitted something, and for all I know it got lost in the mail” is perhaps common for first time submitters. We’d like to see you around the project again, so please don’t take the silence as a cold shoulder.


Mark Dilger
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

#4k.jamison@fujitsu.com
k.jamison@fujitsu.com
In reply to: Mike Lissner (#2)
RE: [Patch]: Documentation of ALTER TABLE re column type changes on binary-coercible fields

On Wednesday, January 29, 2020 3:56 AM (GMT+9), Mike Lissner wrote:

Hi all, I didn't get any replies to this. Is this the right way to send in a patch to the
docs?

Hello,
Yes, although your current patch does not apply as I tried it in my machine.
But you can still rebase it.
For the reviewers/committers to keep track of this, I think it might be better to
register your patch to the commitfest app: https://commitfest.postgresql.org/27/,
and you may put it under the "Documentation" topic.

There's also a CFbot to check online whether your patch still applies cleanly
and passes the tests, especially after several commits in the source code.
Current CF: http://commitfest.cputube.org/index.html
Next CF: http://commitfest.cputube.org/next.html

Regards,
Kirk Jamison

Show quoted text

On Thu, Jan 23, 2020 at 11:01 PM Mike Lissner <mlissner@michaeljaylissner.com
<mailto:mlissner@michaeljaylissner.com> > wrote:

Hi, first patch here and first post to pgsql-hackers. Here goes.

Enclosed please find a patch to tweak the documentation of the ALTER TABLE
page. I believe this patch is ready to be applied to master and backported all the way
to 9.2.

On the ALTER TABLE page, it currently notes that if you change the type of a
column, even to a binary coercible type:

any indexes on the affected columns must still be rebuilt.

It appears this hasn't been true for about eight years, since 367bc426a.

Here's the discussion of the topic from earlier today and yesterday:

https://www.postgresql.org/message-
id/flat/CAMp9%3DExXtH0NeF%2BLTsNrew_oXycAJTNVKbRYnqgoEAT01t%3D67A%40
mail.gmail.com

I haven't run tests, but I presume they'll be unaffected by a documentation
change.

I've made an effort to follow the example of other people's patches I looked
at, but I haven't contributed here before. Happy to take another stab at this if this
doesn't hit the mark — though I hope it does. I love and appreciate Postgresql and
hope that I can do my little part to make it better.

For the moment, I haven't added this to commitfest. I don't know what it is,
but I suspect this is small enough somebody will just pick it up.

Mike

#5Michael Banck
michael.banck@credativ.de
In reply to: Mike Lissner (#1)
Re: Patching documentation of ALTER TABLE re column type changes on binary-coercible fields

Hi,

I've stumbled over this topic today, and found your patch.

On Thu, Jan 23, 2020 at 11:01:36PM -0800, Mike Lissner wrote:

Enclosed please find a patch to tweak the documentation of the ALTER TABLE
page. I believe this patch is ready to be applied to master and backported
all the way to 9.2.

On the ALTER TABLE page, it currently notes that if you change the type of
a column, even to a binary coercible type:

any indexes on the affected columns must still be rebuilt.

It appears this hasn't been true for about eight years, since 367bc426a.

Here's the discussion of the topic from earlier today and yesterday:

/messages/by-id/CAMp9=ExXtH0NeF+LTsNrew_oXycAJTNVKbRYnqgoEAT01t=67A@mail.gmail.com

I haven't run tests, but I presume they'll be unaffected by a documentation
change.

I've made an effort to follow the example of other people's patches I
looked at, but I haven't contributed here before. Happy to take another
stab at this if this doesn't hit the mark — though I hope it does. I love
and appreciate Postgresql and hope that I can do my little part to make it
better.

For the moment, I haven't added this to commitfest. I don't know what it
is, but I suspect this is small enough somebody will just pick it up.

Mike

Index: doc/src/sgml/ref/alter_table.sgml
IDEA additional info:
Subsystem: com.intellij.openapi.diff.impl.patch.CharsetEP
<+>UTF-8
===================================================================
--- doc/src/sgml/ref/alter_table.sgml	(revision 6de7bcb76f6593dcd107a6bfed645f2142bf3225)
+++ doc/src/sgml/ref/alter_table.sgml	(revision 9a813e0896e828900739d95f78b5e4be10dac365)
@@ -1225,10 +1225,9 @@
existing column, if the <literal>USING</literal> clause does not change
the column contents and the old type is either binary coercible to the new
type or an unconstrained domain over the new type, a table rewrite is not
-    needed; but any indexes on the affected columns must still be rebuilt.
-    Table and/or index rebuilds may take a
-    significant amount of time for a large table; and will temporarily require
-    as much as double the disk space.
+    needed. Table and/or index rebuilds may take a significant amount of time
+    for a large table; and will temporarily require as much as double the disk
+    space.
</para>

In general, I find the USING part in that paragraph a bit confusing; I
think the main use case for ALTER COLUMN ... TYPE is without it. So I
would suggest separating the two and make the general case (table and
indexe rewrites are not needed if the type is binary coercible without
having USING in the same sentence.

Michael

--
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax: +49 2166 9901-100
Email: michael.banck@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Sascha Heuer, Geoff Richardson,
Peter Lilley

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz