Re: DOCS: add helpful partitioning links

On Tue, Mar 19, 2024 at 3:08 AM Ashutosh Bapat
<ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>
> Hi Robert,
>
>
> On Mon, Mar 18, 2024 at 10:52 PM Robert Treat <rob(at)xzilla(dot)net> wrote:
>>
>> On Thu, Mar 14, 2024 at 12:15 PM Ashutosh Bapat
>> <ashutosh(dot)bapat(dot)oss(at)gmail(dot)com> wrote:
>> >
>> > Hi Robert,
>> >
>> > On Thu, Mar 7, 2024 at 10:49 PM Robert Treat <rob(at)xzilla(dot)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 think 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 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 break 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 it if 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