Thread: Missing column_constraint explanation
The following documentation comment has been logged on the website: Page: https://www.postgresql.org/docs/9.6/static/sql-altertable.html Description: Missing column_constraint explanation in parameters section
On Wed, Dec 20, 2017 at 6:08 PM, PG Doc comments form <noreply@postgresql.org> wrote: > The following documentation comment has been logged on the website: > > Page: https://www.postgresql.org/docs/9.6/static/sql-altertable.html > Description: > > Missing column_constraint explanation in parameters section Those docs say already that ADD COLUMN follows the same grammar as CREATE TABLE, which basically means that there is no need to duplicate the same definition in two places. Note that the same thing applies to table_constraint. -- Michael
Michael, * Michael Paquier (michael.paquier@gmail.com) wrote: > On Wed, Dec 20, 2017 at 6:08 PM, PG Doc comments form > <noreply@postgresql.org> wrote: > > The following documentation comment has been logged on the website: > > > > Page: https://www.postgresql.org/docs/9.6/static/sql-altertable.html > > Description: > > > > Missing column_constraint explanation in parameters section > > Those docs say already that ADD COLUMN follows the same grammar as > CREATE TABLE, which basically means that there is no need to duplicate > the same definition in two places. Note that the same thing applies to > table_constraint. I actually disagree with this because it means that psql's \h output for ALTER TABLE references column_constraint but doesn't define it anywhere. I'd rather see us move in the other direction- let's try to make the \h output for each command actually stand alone. There was some progress made in that direction recently though I don't recall which command it was for off-hand, but I'd prefer if it was a general rule. Now, if we could do that in such a way that we avoid having to actually duplicate the 'source' for these productions into different places in the documentation, that would be fantastic because it certainly isn't fun having to find all the places that need to be updated, but I'm not sure how easy that would be to do (and to make work with how psql's help is generated...). Thanks! Stephen
Attachment
On Thu, Dec 21, 2017 at 12:15 PM, Stephen Frost <sfrost@snowman.net> wrote: > Now, if we could do that in such a way that we avoid having to actually > duplicate the 'source' for these productions into different places in > the documentation, that would be fantastic because it certainly isn't > fun having to find all the places that need to be updated, but I'm not > sure how easy that would be to do (and to make work with how psql's help > is generated...). You are looking for something like how feature-supported.sgml is handled after its automatic generation, except that in this case you just create a new sgml file which has the definition data you want to load, define it with <!ENTITY blah SYSTEM "blah.sgml">, and then load it using something like an entity &blah; in the CREATE or ALTER TABLE docs. That's a bit of refactoring though, but you could shape it by putting all those lower-level definitions in a subdirectory like sgml/defs or such, avoiding any duplication in those definitions. -- Michael
Michael, * Michael Paquier (michael.paquier@gmail.com) wrote: > On Thu, Dec 21, 2017 at 12:15 PM, Stephen Frost <sfrost@snowman.net> wrote: > > Now, if we could do that in such a way that we avoid having to actually > > duplicate the 'source' for these productions into different places in > > the documentation, that would be fantastic because it certainly isn't > > fun having to find all the places that need to be updated, but I'm not > > sure how easy that would be to do (and to make work with how psql's help > > is generated...). > > You are looking for something like how feature-supported.sgml is > handled after its automatic generation, except that in this case you > just create a new sgml file which has the definition data you want to > load, define it with <!ENTITY blah SYSTEM "blah.sgml">, and then load > it using something like an entity &blah; in the CREATE or ALTER TABLE > docs. That's a bit of refactoring though, but you could shape it by > putting all those lower-level definitions in a subdirectory like > sgml/defs or such, avoiding any duplication in those definitions. I'm not really sure that we want to go there for this case though. Perhaps others disagree, but that seems like a lot to avoid this particular duplication, which really isn't all that bad. This patch also seems to have gotten lost in the shuffle of things, but it still applies cleanly and I took another look at it today and it looks good to me, so I'm going to stick it in the CF and mark it as Needs Review for now. Perhaps someone else can give it another once-over to make sure everything looks good and, if so, mark it as Ready for Committer and then I'll take care of it. Thanks! Stephen
Attachment
On Sat, Jan 13, 2018 at 09:06:22PM -0500, Stephen Frost wrote: > I'm not really sure that we want to go there for this case though. > Perhaps others disagree, but that seems like a lot to avoid this > particular duplication, which really isn't all that bad. > > This patch also seems to have gotten lost in the shuffle of things, but > it still applies cleanly and I took another look at it today and it > looks good to me, so I'm going to stick it in the CF and mark it as > Needs Review for now. Perhaps someone else can give it another > once-over to make sure everything looks good and, if so, mark it as > Ready for Committer and then I'll take care of it. I may be missing something, but no patch is attached to this thread. Honestly I think that duplicating this code should be avoided, and the patch to produce is not that complicated technically. So are you planning to just duplicate hte definitions in CREATE TABLE to ALTER TABLE? This is a recipy for forgetting updates in the future on one page or the other... -- Michael
Attachment
Michael, * Michael Paquier (michael.paquier@gmail.com) wrote: > On Sat, Jan 13, 2018 at 09:06:22PM -0500, Stephen Frost wrote: > > I'm not really sure that we want to go there for this case though. > > Perhaps others disagree, but that seems like a lot to avoid this > > particular duplication, which really isn't all that bad. > > > > This patch also seems to have gotten lost in the shuffle of things, but > > it still applies cleanly and I took another look at it today and it > > looks good to me, so I'm going to stick it in the CF and mark it as > > Needs Review for now. Perhaps someone else can give it another > > once-over to make sure everything looks good and, if so, mark it as > > Ready for Committer and then I'll take care of it. > > I may be missing something, but no patch is attached to this > thread. Honestly I think that duplicating this code should be avoided, > and the patch to produce is not that complicated technically. So are you > planning to just duplicate hte definitions in CREATE TABLE to ALTER > TABLE? This is a recipy for forgetting updates in the future on one > page or the other... I'm not really sure why the thread keeps getting broken, but the original was here: https://www.postgresql.org/message-id/flat/CAB_COdgOoA=G18RhWPoW8zZ+xOxTns7xD7psHA=ct+XccokbUg@mail.gmail.com#CAB_COdgOoA=G18RhWPoW8zZ+xOxTns7xD7psHA=ct+XccokbUg@mail.gmail.com where the latest patch is from Amit. I agree that there's some risk that someone changing what 'column_constraint' means will miss changing it everywhere it needs to be changed in the documentation, but that's hardly new, we have quite a few places that need to be changed when someone adds such a new feature. We also do not actually have the exact same definition of it everywhere either- see CREATE TABLE and CREATE FOREIGN TABLE for example. In this case, column_constraint is the same between CREATE TABLE and ALTER TABLE, and presumably the same is true between CREATE FOREIGN TABLE and ALTER FOREIGN TABLE, but it's not the same between regular tables and foreign tables, despite sharing the same name. As such, someone extending what column_constraint means would need to at least be thinking about if their new feature needs to be added into the CREATE FOREIGN TABLE definition too. Unless we start marking up the documentation somehow further to indicate which bits of column_constraint are accepted for regular tables and which are accepted for foreign tables, but we may also have cases where one part of 'column_constraint' is actually different between the two for perhaps entirely appropriate technical reasons. Further, as mentioned upthread in https://www.postgresql.org/message-id/20171221031511.GX4628%40tamriel.snowman.net I'm concerned that this wouldn't be very easy to make work with the way psql's documentation is generated. Did you take a look at how that works with create_help.pl? I don't see create_help.pl as supporting 'entity'. We could perhaps fix that, but I'm not really sure it's worth it and it's certainly quite a bit to add on to this particular patch. If we used the method that features-supported.sgml uses, where we have another perl script that's used to build the sgml files up from text files, then we'd also need to make building psql depend on the docs having been built, I believe, and I don't think we really want to do that either. I wouldn't be against a larger patch which actually refactored all of these sub-stanzas into independent files (with unique names, unlike how column_constraint is today) and also taught create_help.pl and the doc build to be able to work with them, but that's a great deal more work and nothing in this patch stops that from being done in the future. In the end, I'm inclinced to simply adopt this change and even consider back-patching it as a documentation fix. Thanks! Stephen
Attachment
On Sun, Jan 14, 2018 at 09:33:13AM -0500, Stephen Frost wrote: > I'm not really sure why the thread keeps getting broken, but the > original was here: > > https://www.postgresql.org/message-id/flat/CAB_COdgOoA=G18RhWPoW8zZ+xOxTns7xD7psHA=ct+XccokbUg@mail.gmail.com#CAB_COdgOoA=G18RhWPoW8zZ+xOxTns7xD7psHA=ct+XccokbUg@mail.gmail.com > > where the latest patch is from Amit. Indeed I missed this one, thanks. This one is a new thread, where somebody has commented directly on the docs of the website. > Further, as mentioned upthread in > https://www.postgresql.org/message-id/20171221031511.GX4628%40tamriel.snowman.net > I'm concerned that this wouldn't be very easy to make work with the way > psql's documentation is generated. Did you take a look at how that > works with create_help.pl? I don't see create_help.pl as supporting > 'entity'. We could perhaps fix that, but I'm not really sure it's worth > it and it's certainly quite a bit to add on to this particular patch. Good point here. -- Michael