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. -- Best Wishes, Ashutosh Bapat
