On Tue, Mar 19, 2024 at 3:08 AM Ashutosh Bapat <ashutosh.bapat....@gmail.com> wrote: > > Hi Robert, > > > On Mon, Mar 18, 2024 at 10:52 PM Robert Treat <r...@xzilla.net> wrote: >> >> On Thu, Mar 14, 2024 at 12:15 PM Ashutosh Bapat >> <ashutosh.bapat....@gmail.com> wrote: >> > >> > Hi Robert, >> > >> > On Thu, Mar 7, 2024 at 10:49 PM Robert Treat <r...@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 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
improve-partition-links_v4.patch
Description: Binary data
