Thread: doc: CREATE FOREIGN TABLE .. PARTITION OF .. DEFAULT
It looks like the docs weren't updated in 6f6b99d13 for v11.
The docs also seem to omit "FOR VALUES" literal.
And don't define partition_bound_spec (which I didn't fix here).
diff --git a/doc/src/sgml/ref/create_foreign_table.sgml b/doc/src/sgml/ref/create_foreign_table.sgml
index b374d8645db..1f1c4a52a2a 100644
--- a/doc/src/sgml/ref/create_foreign_table.sgml
+++ b/doc/src/sgml/ref/create_foreign_table.sgml
@@ -35,7 +35,8 @@ CREATE FOREIGN TABLE [ IF NOT EXISTS ] <replaceable class="parameter">table_name
{ <replaceable class="parameter">column_name</replaceable> [ WITH OPTIONS ] [ <replaceable
class="parameter">column_constraint</replaceable>[ ... ] ]
| <replaceable>table_constraint</replaceable> }
[, ... ]
-) ] <replaceable class="parameter">partition_bound_spec</replaceable>
+) ]
+ { FOR VALUES <replaceable class="parameter">partition_bound_spec</replaceable> | DEFAULT }
SERVER <replaceable class="parameter">server_name</replaceable>
[ OPTIONS ( <replaceable class="parameter">option</replaceable> '<replaceable class="parameter">value</replaceable>'
[,... ] ) ]
On Sat, May 21, 2022 at 9:09 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> It looks like the docs weren't updated in 6f6b99d13 for v11.
In my defense, that commit definitely contained documentation changes.
It updated alter_table.sgml and create_table.sgml. I guess we missed
create_foreign_table.sgml, though.
> The docs also seem to omit "FOR VALUES" literal.
> And don't define partition_bound_spec (which I didn't fix here).
>
> diff --git a/doc/src/sgml/ref/create_foreign_table.sgml b/doc/src/sgml/ref/create_foreign_table.sgml
> index b374d8645db..1f1c4a52a2a 100644
> --- a/doc/src/sgml/ref/create_foreign_table.sgml
> +++ b/doc/src/sgml/ref/create_foreign_table.sgml
> @@ -35,7 +35,8 @@ CREATE FOREIGN TABLE [ IF NOT EXISTS ] <replaceable class="parameter">table_name
> { <replaceable class="parameter">column_name</replaceable> [ WITH OPTIONS ] [ <replaceable
class="parameter">column_constraint</replaceable>[ ... ] ]
> | <replaceable>table_constraint</replaceable> }
> [, ... ]
> -) ] <replaceable class="parameter">partition_bound_spec</replaceable>
> +) ]
> + { FOR VALUES <replaceable class="parameter">partition_bound_spec</replaceable> | DEFAULT }
> SERVER <replaceable class="parameter">server_name</replaceable>
> [ OPTIONS ( <replaceable class="parameter">option</replaceable> '<replaceable class="parameter">value</replaceable>'
[,... ] ) ]
OK, makes sense. I guess we need to copy over the definition of
partition_bound_spec from create_table.sgml here as well.
--
Robert Haas
EDB: http://www.enterprisedb.com
On Wed, May 25, 2022 at 9:44 PM Robert Haas <robertmhaas@gmail.com> wrote:
> On Sat, May 21, 2022 at 9:09 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > It looks like the docs weren't updated in 6f6b99d13 for v11.
>
> In my defense, that commit definitely contained documentation changes.
> It updated alter_table.sgml and create_table.sgml. I guess we missed
> create_foreign_table.sgml, though.
>
> > The docs also seem to omit "FOR VALUES" literal.
That would be my mistake.
> > And don't define partition_bound_spec (which I didn't fix here).
> >
> > diff --git a/doc/src/sgml/ref/create_foreign_table.sgml b/doc/src/sgml/ref/create_foreign_table.sgml
> > index b374d8645db..1f1c4a52a2a 100644
> > --- a/doc/src/sgml/ref/create_foreign_table.sgml
> > +++ b/doc/src/sgml/ref/create_foreign_table.sgml
> > @@ -35,7 +35,8 @@ CREATE FOREIGN TABLE [ IF NOT EXISTS ] <replaceable class="parameter">table_name
> > { <replaceable class="parameter">column_name</replaceable> [ WITH OPTIONS ] [ <replaceable
class="parameter">column_constraint</replaceable>[ ... ] ]
> > | <replaceable>table_constraint</replaceable> }
> > [, ... ]
> > -) ] <replaceable class="parameter">partition_bound_spec</replaceable>
> > +) ]
> > + { FOR VALUES <replaceable class="parameter">partition_bound_spec</replaceable> | DEFAULT }
> > SERVER <replaceable class="parameter">server_name</replaceable>
> > [ OPTIONS ( <replaceable class="parameter">option</replaceable> '<replaceable
class="parameter">value</replaceable>'[, ... ] ) ]
>
> OK, makes sense. I guess we need to copy over the definition of
> partition_bound_spec from create_table.sgml here as well.
Yes. a2a2205761 did that for alter_table.sgml and we evidently missed
including create_foreign_table.sgml in that discussion.
Attached 2 patches -- one for PG 11 onwards and another for PG 10.
--
Thanks, Amit Langote
EDB: http://www.enterprisedb.com
Attachment
On Thu, May 26, 2022 at 1:50 AM Amit Langote <amitlangote09@gmail.com> wrote: > Attached 2 patches -- one for PG 11 onwards and another for PG 10. Committed, except I adjusted the v11 version so that the CREATE FOREIGN TABLE documentation would match the CREATE TABLE documentation in that branch. -- Robert Haas EDB: http://www.enterprisedb.com
On Fri, May 27, 2022 at 1:58 AM Robert Haas <robertmhaas@gmail.com> wrote: > On Thu, May 26, 2022 at 1:50 AM Amit Langote <amitlangote09@gmail.com> wrote: > > Attached 2 patches -- one for PG 11 onwards and another for PG 10. > > Committed, except I adjusted the v11 version so that the CREATE > FOREIGN TABLE documentation would match the CREATE TABLE documentation > in that branch. Thank you. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
On Fri, May 27, 2022 at 1:58 AM Robert Haas <robertmhaas@gmail.com> wrote: > Committed, except I adjusted the v11 version so that the CREATE > FOREIGN TABLE documentation would match the CREATE TABLE documentation > in that branch. I think we should fix the syntax synopsis in the Parameters section of the CREATE FOREIGN TABLE reference page as well. Attached is a patch for that. Sorry for being late for this. Best regards, Etsuro Fujita
Attachment
On Fri, May 27, 2022 at 7:15 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > On Fri, May 27, 2022 at 1:58 AM Robert Haas <robertmhaas@gmail.com> wrote: > > Committed, except I adjusted the v11 version so that the CREATE > > FOREIGN TABLE documentation would match the CREATE TABLE documentation > > in that branch. > > I think we should fix the syntax synopsis in the Parameters section > of the CREATE FOREIGN TABLE reference page as well. Oops, good catch. > Attached is a patch for that. Thank you. I think we should also rewrite the description to match the CREATE TABLE's text, as in the attached updated patch. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
Attachment
Amit-san, On Fri, May 27, 2022 at 9:22 PM Amit Langote <amitlangote09@gmail.com> wrote: > On Fri, May 27, 2022 at 7:15 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > > Attached is a patch for that. > I think we should also rewrite the description to match the CREATE > TABLE's text, as in the attached updated patch. Actually, I thought the description would be OK as-is, because it says “See the similar form of CREATE TABLE for more details”, but I agree with you; it’s much better to also rewrite the description as you suggest. I’ll commit the patch unless Robert wants to. Thanks for the review and patch! Best regards, Etsuro Fujita
On Mon, May 30, 2022 at 2:27 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > On Fri, May 27, 2022 at 9:22 PM Amit Langote <amitlangote09@gmail.com> wrote: > > On Fri, May 27, 2022 at 7:15 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > > > Attached is a patch for that. > > > I think we should also rewrite the description to match the CREATE > > TABLE's text, as in the attached updated patch. > > Actually, I thought the description would be OK as-is, because it says > “See the similar form of CREATE TABLE for more details”, but I agree > with you; it’s much better to also rewrite the description as you > suggest. I would probably just update the synopsis. It's not very hard to figure out what's likely to happen even without clicking through the link, so it seems like it's just being long-winded to duplicate the stuff here. But I don't care much if you feel otherwise. > I’ll commit the patch unless Robert wants to. Please go ahead. -- Robert Haas EDB: http://www.enterprisedb.com
On Tue, May 31, 2022 at 9:35 PM Robert Haas <robertmhaas@gmail.com> wrote: > On Mon, May 30, 2022 at 2:27 AM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > > On Fri, May 27, 2022 at 9:22 PM Amit Langote <amitlangote09@gmail.com> wrote: > > > On Fri, May 27, 2022 at 7:15 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > > > > Attached is a patch for that. > > > > > I think we should also rewrite the description to match the CREATE > > > TABLE's text, as in the attached updated patch. > > > > Actually, I thought the description would be OK as-is, because it says > > “See the similar form of CREATE TABLE for more details”, but I agree > > with you; it’s much better to also rewrite the description as you > > suggest. > > I would probably just update the synopsis. It's not very hard to > figure out what's likely to happen even without clicking through the > link, so it seems like it's just being long-winded to duplicate the > stuff here. But I don't care much if you feel otherwise. It looks like there are pros and cons. I think it’s a matter of preference, though. I thought it would be an improvement, but I agree that we can live without it, so I changed my mind; I'll go with my version. I think we could revisit this later. > > I’ll commit the patch unless Robert wants to. > > Please go ahead. OK Thanks! Best regards, Etsuro Fujita
On Wed, Jun 1, 2022 at 6:15 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > On Tue, May 31, 2022 at 9:35 PM Robert Haas <robertmhaas@gmail.com> wrote: > > I would probably just update the synopsis. It's not very hard to > > figure out what's likely to happen even without clicking through the > > link, so it seems like it's just being long-winded to duplicate the > > stuff here. But I don't care much if you feel otherwise. > > It looks like there are pros and cons. I think it’s a matter of > preference, though. > > I thought it would be an improvement, but I agree that we can live > without it, so I changed my mind; I'll go with my version. I think we > could revisit this later. I guess I'm fine with leaving the text as-is, though slightly bothered by leaving the phrase "partition of the given parent table with specified partition bound values" to also cover the DEFAULT partition case. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com
On Thu, Jun 2, 2022 at 10:23 AM Amit Langote <amitlangote09@gmail.com> wrote: > On Wed, Jun 1, 2022 at 6:15 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > > On Tue, May 31, 2022 at 9:35 PM Robert Haas <robertmhaas@gmail.com> wrote: > > > I would probably just update the synopsis. It's not very hard to > > > figure out what's likely to happen even without clicking through the > > > link, so it seems like it's just being long-winded to duplicate the > > > stuff here. But I don't care much if you feel otherwise. > > > > It looks like there are pros and cons. I think it’s a matter of > > preference, though. > > > > I thought it would be an improvement, but I agree that we can live > > without it, so I changed my mind; I'll go with my version. I think we > > could revisit this later. > > I guess I'm fine with leaving the text as-is, though slightly bothered > by leaving the phrase "partition of the given parent table with > specified partition bound values" to also cover the DEFAULT partition > case. I think we should discuss this separately, maybe as a HEAD-only patch, so I pushed my version, leaving the description as-is. Best regards, Etsuro Fujita
On Thu, Jun 2, 2022 at 6:14 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > On Thu, Jun 2, 2022 at 10:23 AM Amit Langote <amitlangote09@gmail.com> wrote: > > On Wed, Jun 1, 2022 at 6:15 PM Etsuro Fujita <etsuro.fujita@gmail.com> wrote: > > > On Tue, May 31, 2022 at 9:35 PM Robert Haas <robertmhaas@gmail.com> wrote: > > > > I would probably just update the synopsis. It's not very hard to > > > > figure out what's likely to happen even without clicking through the > > > > link, so it seems like it's just being long-winded to duplicate the > > > > stuff here. But I don't care much if you feel otherwise. > > > > > > It looks like there are pros and cons. I think it’s a matter of > > > preference, though. > > > > > > I thought it would be an improvement, but I agree that we can live > > > without it, so I changed my mind; I'll go with my version. I think we > > > could revisit this later. > > > > I guess I'm fine with leaving the text as-is, though slightly bothered > > by leaving the phrase "partition of the given parent table with > > specified partition bound values" to also cover the DEFAULT partition > > case. > > I think we should discuss this separately, maybe as a HEAD-only patch, > so I pushed my version, leaving the description as-is. No problem, thanks for the fix. -- Thanks, Amit Langote EDB: http://www.enterprisedb.com