doc: CREATE FOREIGN TABLE .. PARTITION OF .. DEFAULT

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

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

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

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