Michael, * Michael Paquier (michael.paqu...@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+xccok...@mail.gmail.com#CAB_COdgOoA=G18RhWPoW8zZ+xOxTns7xD7psHA=ct+xccok...@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
signature.asc
Description: PGP signature
