Обсуждение: DOCS: add helpful partitioning links

This patch adds a link to the "attach partition" command section
(similar to the detach partition link above it) as well as a link to
"create table like" as both commands contain additional information
that users should review beyond what is laid out in this section.
There's also a couple of wordsmiths in nearby areas to improve
readability.

Robert Treat
https://xzilla.net

On Thu, Mar 14, 2024 at 12:15 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Hi Robert,
>
> On Thu, Mar 7, 2024 at 10:49 PM Robert Treat <rob@xzilla.net> wrote:
>>
>> This patch adds a link to the "attach partition" command section
>> (similar to the detach partition link above it) as well as a link to
>> "create table like" as both commands contain additional information
>> that users should review beyond what is laid out in this section.
>> There's also a couple of wordsmiths in nearby areas to improve
>> readability.
>
>
> Thanks.
>
> The patch gives error when building html
>
> ddl.sgml:4300: element link: validity error : No declaration for attribute linked of element link
>      <link linked="sql-createtable-parms-like"><literal>CREATE TABLE ... LIKE</l
>                                               ^
> ddl.sgml:4300: element link: validity error : Element link does not carry attribute linkend
> nked="sql-createtable-parms-like"><literal>CREATE TABLE ... LIKE</literal></link
>                                                                                ^
> make[1]: *** [Makefile:72: postgres-full.xml] Error 4
> make[1]: *** Deleting file 'postgres-full.xml'
> make[1]: Leaving directory '/home/ashutosh/work/units/pg_review/coderoot/pg/doc/src/sgml'
> make: *** [Makefile:8: html] Error 2
>
> I have fixed in the attached.
>

Doh! Thanks!

>
> -     As an alternative, it is sometimes more convenient to create the
> -     new table outside the partition structure, and attach it as a
> +     As an alternative, it is sometimes more convenient to create a
> +     new table outside of the partition structure, and attach it as a
>
> it uses article "the" for "new table" since it's referring to the partition mentioned in the earlier example. I don't
thinkusing "a" is correct. 
>

I think this section has a general problem of mingling the terms table
and partition in a way they can be confusing.

In this case, you have to infer that the term "the new table" is
referring not to the only table mentioned in the previous paragraph
(the partitioned table), but actually to the partition mentioned in
the previous paragraph. For long term postgres folks the idea that
partitions are tables isn't a big deal, but that isn't how folks
coming from other databases see things. So I lean towards "a new
table" because we are specifically talking about an alternative to the
above paragraph... ie. don't make a new partition, make a new table.
And tbh I think that wording (create a new table and attach it as a
partition) is still better than the wording in your patch, because the
"new partition" you are creating isn't a partition until it is
attached; it is just a new table.

> "outside" seems better than "outside of". See https://english.stackexchange.com/questions/9700/outside-or-outside-of.
ButI think the meaning of the sentence will be more clear if we rephrase it as in the attached patch. 
>

This didn't really clarify anything for me, as the discussion in that
link seems to be around the usage of the term wrt physical location,
and it is much less clear about the context of a logical construct.
Granted, your patch removes that, though I think now I'd lean toward
using the phrase "separate from".

> -     convenient, as not only will the existing partitions become indexed, but
> -     also any partitions that are created in the future will.  One limitation is
> +     convenient as not only will the existing partitions become indexed, but
> +     any partitions created in the future will as well.  One limitation is
>
> I am finding the current construct hard to read. The comma is misplaced as you have pointed out. The pair of commas
breakthe "not only" ... "but also" construct. I have tried to simplify the sentence in the attached. Please review. 
>
> -     the partitioned table; such an index is marked invalid, and the partitions
> -     do not get the index applied automatically.  The indexes on partitions can
> -     be created individually using <literal>CONCURRENTLY</literal>, and then
> +     the partitioned table; such an index is marked invalid and the partitions
> +     do not get the index applied automatically.  The partition indexes can
>
> "indexes on partition" is clearer than "partition index". Fixed in the attached patch.
>
> Please review.

The language around all this is certainly tricky (like, what is a
partitioned index vs parent index?), and one thing I'd certainly try
to avoid is using any words like "inherited" which is also overloaded
in this context. In any case, I took in all the above and had a stab
at a v3

Robert Treat
https://xzilla.net

On Tue, Mar 19, 2024 at 3:08 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
>
> Hi Robert,
>
>
> On Mon, Mar 18, 2024 at 10:52 PM Robert Treat <rob@xzilla.net> wrote:
>>
>> On Thu, Mar 14, 2024 at 12:15 PM Ashutosh Bapat
>> <ashutosh.bapat.oss@gmail.com> wrote:
>> >
>> > Hi Robert,
>> >
>> > On Thu, Mar 7, 2024 at 10:49 PM Robert Treat <rob@xzilla.net> wrote:
>> >>
>> >> This patch adds a link to the "attach partition" command section
>> >> (similar to the detach partition link above it) as well as a link to
>> >> "create table like" as both commands contain additional information
>> >> that users should review beyond what is laid out in this section.
>> >> There's also a couple of wordsmiths in nearby areas to improve
>> >> readability.
>> >
>> >
>> > Thanks.
>> >
>> > The patch gives error when building html
>> >
>> > ddl.sgml:4300: element link: validity error : No declaration for attribute linked of element link
>> >      <link linked="sql-createtable-parms-like"><literal>CREATE TABLE ... LIKE</l
>> >                                               ^
>> > ddl.sgml:4300: element link: validity error : Element link does not carry attribute linkend
>> > nked="sql-createtable-parms-like"><literal>CREATE TABLE ... LIKE</literal></link
>> >                                                                                ^
>> > make[1]: *** [Makefile:72: postgres-full.xml] Error 4
>> > make[1]: *** Deleting file 'postgres-full.xml'
>> > make[1]: Leaving directory '/home/ashutosh/work/units/pg_review/coderoot/pg/doc/src/sgml'
>> > make: *** [Makefile:8: html] Error 2
>> >
>> > I have fixed in the attached.
>> >
>>
>> Doh! Thanks!
>>
>> >
>> > -     As an alternative, it is sometimes more convenient to create the
>> > -     new table outside the partition structure, and attach it as a
>> > +     As an alternative, it is sometimes more convenient to create a
>> > +     new table outside of the partition structure, and attach it as a
>> >
>> > it uses article "the" for "new table" since it's referring to the partition mentioned in the earlier example. I
don'tthink using "a" is correct. 
>> >
>>
>> I think this section has a general problem of mingling the terms table
>> and partition in a way they can be confusing.
>>
>> In this case, you have to infer that the term "the new table" is
>> referring not to the only table mentioned in the previous paragraph
>> (the partitioned table), but actually to the partition mentioned in
>> the previous paragraph. For long term postgres folks the idea that
>> partitions are tables isn't a big deal, but that isn't how folks
>> coming from other databases see things. So I lean towards "a new
>> table" because we are specifically talking about an alternative to the
>> above paragraph... ie. don't make a new partition, make a new table.
>> And tbh I think that wording (create a new table and attach it as a
>> partition) is still better than the wording in your patch, because the
>> "new partition" you are creating isn't a partition until it is
>> attached; it is just a new table.
>>
>> > "outside" seems better than "outside of". See
https://english.stackexchange.com/questions/9700/outside-or-outside-of.But I think the meaning of the sentence will be
moreclear if we rephrase it as in the attached patch. 
>> >
>>
>> This didn't really clarify anything for me, as the discussion in that
>> link seems to be around the usage of the term wrt physical location,
>> and it is much less clear about the context of a logical construct.
>> Granted, your patch removes that, though I think now I'd lean toward
>> using the phrase "separate from".
>>
>> > -     convenient, as not only will the existing partitions become indexed, but
>> > -     also any partitions that are created in the future will.  One limitation is
>> > +     convenient as not only will the existing partitions become indexed, but
>> > +     any partitions created in the future will as well.  One limitation is
>> >
>> > I am finding the current construct hard to read. The comma is misplaced as you have pointed out. The pair of
commasbreak the "not only" ... "but also" construct. I have tried to simplify the sentence in the attached. Please
review.
>> >
>> > -     the partitioned table; such an index is marked invalid, and the partitions
>> > -     do not get the index applied automatically.  The indexes on partitions can
>> > -     be created individually using <literal>CONCURRENTLY</literal>, and then
>> > +     the partitioned table; such an index is marked invalid and the partitions
>> > +     do not get the index applied automatically.  The partition indexes can
>> >
>> > "indexes on partition" is clearer than "partition index". Fixed in the attached patch.
>> >
>> > Please review.
>>
>> The language around all this is certainly tricky (like, what is a
>> partitioned index vs parent index?), and one thing I'd certainly try
>> to avoid is using any words like "inherited" which is also overloaded
>> in this context. In any case, I took in all the above and had a stab
>> at a v3
>>
>
> The patch doesn't apply cleanly
> $ git apply /tmp/improve-partition-links_v3.patch
> error: patch failed: doc/src/sgml/ddl.sgml:4266
> error: doc/src/sgml/ddl.sgml: patch does not apply
>
> $ patch -p1 < /tmp/improve-partition-links_v3.patch
> patching file doc/src/sgml/ddl.sgml
> Hunk #1 FAILED at 4266.
> Hunk #2 succeeded at 4333 (offset 12 lines).
> Hunk #3 FAILED at 4332.
> 2 out of 3 hunks FAILED -- saving rejects to file doc/src/sgml/ddl.sgml.rej
>
> +     As an alternative to creating new partitions, it is sometimes more
>
> edit: creating a new partition .. rest of the sentence is in singular.
>
> +     convenient to create a new table seperate from the partition structure
> +     and attach it as a partition later. This allows new data to be loaded,
> +     checked, and transformed prior to it appearing in the partitioned table.
>
> Rest of it looks good to me.
>
> Please add it to the next commitfest. Most likely the patch will be considered for PG 17 itself, but we won't forget
itif it's in CF. 
>

I've put it in the next commitfest with target version of 17, and I've
added you as a reviewer :-)

Also, attached is an updated patch with your change above which should
apply cleanly to the current git master.

Thanks again,

Robert Treat
https://xzilla.net

On Thu, Mar 21, 2024 at 7:27 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> On Wed, Mar 20, 2024 at 5:22 PM Ashutosh Bapat <ashutosh.bapat.oss@gmail.com> wrote:
>> On Tue, Mar 19, 2024 at 6:38 PM Robert Treat <rob@xzilla.net> wrote:
>>>
>>>
>>> I've put it in the next commitfest with target version of 17, and I've
>>> added you as a reviewer :-)
>>>
>>
>> Thanks.
>>
>>>
>>> Also, attached is an updated patch with your change above which should
>>> apply cleanly to the current git master.
>>
>>
>> It did apply for me now.
>>
>> The HTML renders good, the links work as expected.
>>
>> The CREATE TABLE ... LIKE command
>> I think the original word "option" instead of "command" is better since we are talking about LIKE as an option to
CREATETABLE instead of CREATE TABLE command. 
>>
>> +     but any future attached or created partitions will be indexed as well.
>>
>> I think just "any future partitions will be indexed as well" would suffice, no need to mention whether they were
createdor attached. 
>>
>> +     One limitation when creating new indexes on partitioned tables is that it
>> +     is not possible to use the <literal>CONCURRENTLY</literal>
>> +     qualifier when creating such a partitioned index.  To avoid long
>>
>> The sentence uses two different phrases, "indexes on partitioned tables" and "partitioned index", for the same thing
inthe same sentence. Probably it is better to leave original sentence as is. 
>>
>> But I think it's time for a committer to take a look at this. Please feel free to address the above comments if you
agreewith them. Marking this as ready for committer. 
>>
>
> The patch changes things not directly related to $Subject. It will be good to add a commit message to the patch
describingwhat are those changes about. I observe that all of them are in section "partition maintenance".
https://www.postgresql.org/docs/16/ddl-partitioning.html#DDL-PARTITIONING-DECLARATIVE-MAINTENANCE.Do you see any more
editsrequired in that section? 
>

Heh, well, I had thought about some other possible improvements to
that section but hadn't quite worked them out, but you inspired me to
have another go of it ;-)

v5 patch attached which I think further improves clarity/brevity of
this section. I've left the patch name the same for simplicity, but
I'd agree that the commit would now be more along the lines of editing
/ improvements / copyrighting of "Partition Maintenance" docs.

Robert Treat
https://xzilla.net

On Mon, Mar 25, 2024 at 6:43 AM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> On Fri, Mar 22, 2024 at 10:58 PM Robert Treat <rob@xzilla.net> wrote:
>> v5 patch attached which I think further improves clarity/brevity of
>> this section. I've left the patch name the same for simplicity, but
>> I'd agree that the commit would now be more along the lines of editing
>> / improvements / copyrighting of "Partition Maintenance" docs.
>
>
> Right. Minor suggestions.
>
> -     It is recommended to drop the now-redundant <literal>CHECK</literal>
> -     constraint after the <command>ATTACH PARTITION</command> is complete.  If
> -     the table being attached is itself a partitioned table, then each of its
> +     As illustrated above, it is recommended to avoid this scan by creating a
> +     <literal>CHECK</literal> constraint on the to be attached table that
>
> Instead of "to be attached table", "table to be attached" reads better. You may want to add "as a partition" after
that.
>

That sounds more awkward to me, but I've done some rewording to avoid both.

>       Similarly, if the partitioned table has a <literal>DEFAULT</literal>
>       partition, it is recommended to create a <literal>CHECK</literal>
>       constraint which excludes the to-be-attached partition's constraint.  If
> -     this is not done then the <literal>DEFAULT</literal> partition will be
> +     this is not done, the <literal>DEFAULT</literal> partition must be
>
> I am not sure whether replacing "will" by "must" is correct. Usually I have seen "will" being used in such sentences,
"must"seems appropriate given the necessity. 
>

OK

Updated patch attached.


Robert Treat
https://xzilla.net

On 2024-Mar-28, Ashutosh Bapat wrote:

> LGTM.
> 
> The commitfest entry is marked as RFC already.
> 
> Thanks for taking care of the comments.

Thanks for reviewing.  I noticed a typo "seperate", fixed here.  Also, I
noticed that Robert added an empty line which looks in the source like
he's breaking the paragraph -- but because he didn't add a closing </para> 
and an opening <para> to the next one, there's no actual new paragraph
in the HTML output.

My first instinct was to add those.  However, upon reading the text, I
noticed that the previous paragraph ends without offering an example,
and then we attach the example to the paragraph that takes about CREATE
TABLE LIKE showing both techniques, which seemed a bit odd.  So instead
I joined both paragraphs back together.  I'm unsure which one looks
better.  Which one do you vote for?

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
"No hay ausente sin culpa ni presente sin disculpa" (Prov. francés)

On Thu, Mar 28, 2024 at 11:43 PM Ashutosh Bapat
<ashutosh.bapat.oss@gmail.com> wrote:
> On Thu, Mar 28, 2024 at 11:22 PM Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
>>
>> On 2024-Mar-28, Ashutosh Bapat wrote:
>>
>> > LGTM.
>> >
>> > The commitfest entry is marked as RFC already.
>> >
>> > Thanks for taking care of the comments.
>>
>> Thanks for reviewing.  I noticed a typo "seperate", fixed here.
>
>
> Thanks for catching it.
>
>>
>> Also, I
>> noticed that Robert added an empty line which looks in the source like
>> he's breaking the paragraph -- but because he didn't add a closing </para>
>> and an opening <para> to the next one, there's no actual new paragraph
>> in the HTML output.
>>
>>
>> My first instinct was to add those.  However, upon reading the text, I
>> noticed that the previous paragraph ends without offering an example,
>> and then we attach the example to the paragraph that takes about CREATE
>> TABLE LIKE showing both techniques, which seemed a bit odd.  So instead
>> I joined both paragraphs back together.  I'm unsure which one looks
>> better.  Which one do you vote for?
>
>
> "CREATE TABLE ... LIKE" is mentioned in a separate paragraph in HEAD as well. The confused me too but I didn't find
anyreason. Robert just made that explicit by adding a blank line. I thought that was ok. But it makes sense to not have
aseparate paragraph in the source code too. Thanks for fixing it. I think the intention of the current code as well as
thepatch is to have a single paragraph in HTML output, same as "no-extra-para" output. 
>

It does seem like the source and the html output ought to match, so +1 from me.


Robert Treat
https://xzilla.net