Обсуждение: 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

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

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

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

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