CREATE PUBLICATION with 'publish_generated_columns' parameter specified but unassigned
Hi,
Recently, I learned it is possible to say:
CREATE PUBLICATION pub ... WITH (publish_generated_columns);
This is equivalent to:
CREATE PUBLICATION pub ... WITH (publish_generated_columns = stored);
~~~
Example:
postgres=# create publication pub for table t with
(publish_generated_columns=xxx);
ERROR: invalid value for publication parameter
"publish_generated_columns": "xxx"
DETAIL: Valid values are "none" and "stored".
postgres=# create publication pub for table t with (publish_generated_columns);
CREATE PUBLICATION
postgres=# \dRp pub
List of publications
Name | Owner | All tables | Inserts | Updates | Deletes | Truncates
| Generated columns | Via root
------+----------+------------+---------+---------+---------+-----------+-------------------+----------
pub | postgres | f | t | t | t | t
| stored | f
(1 row)
~~~
However, I could not find this kind of shorthand specification (having
no assigned value) mentioned anywhere in the PG docs [1]https://www.postgresql.org/docs/devel/sql-createpublication.html.
OTOH, I did see this is being tested/commented in 'publication.sql'
[2]: https://github.com/postgres/postgres/blob/master/src/test/regress/sql/publication.sql#L1193
~~~
What is the verdict for this syntax -- ok or not?
a- Is it some deliberate, undocumented shorthand?
b- Is it just a documentation omission?
c- Is this an unintended quirk? IIRC, this parameter was initially
Boolean during development, before it was changed to an enum. Maybe
this behaviour/test is an accidental hangover from then?
======
[1]: https://www.postgresql.org/docs/devel/sql-createpublication.html
[2]: https://github.com/postgres/postgres/blob/master/src/test/regress/sql/publication.sql#L1193
Kind Regards,
Peter Smith.
Fujitsu Australia
On Sunday, August 3, 2025, Peter Smith <smithpb2250@gmail.com> wrote:
Recently, I learned it is possible to say:
CREATE PUBLICATION pub ... WITH (publish_generated_columns);
This is equivalent to:
CREATE PUBLICATION pub ... WITH (publish_generated_columns = stored);What is the verdict for this syntax -- ok or not?
Not.
An enum should not allow for an omitted value. The documented policy of
only booleans being allowed an optional value is what is expected. I’d say
this is a new-in-18 bug that should be fixed in the code. The
documentation is correct - absence of the option means “none”, presence
requires an explicit value and not its own missing-value default.
David J.
On Mon, Aug 4, 2025 at 4:37 PM David G. Johnston
<david.g.johnston@gmail.com> wrote:
On Sunday, August 3, 2025, Peter Smith <smithpb2250@gmail.com> wrote:
Recently, I learned it is possible to say:
CREATE PUBLICATION pub ... WITH (publish_generated_columns);
This is equivalent to:
CREATE PUBLICATION pub ... WITH (publish_generated_columns = stored);What is the verdict for this syntax -- ok or not?
Not.
An enum should not allow for an omitted value. The documented policy of only booleans being allowed an optional value is what is expected. I’d say this is a new-in-18 bug that should be fixed in the code. The documentation is correct - absence of the option means “none”, presence requires an explicit value and not its own missing-value default.
Thanks. That is the same as my understanding.
I will post a patch to do this tomorrow.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
On Mon, Aug 4, 2025 at 11:54 AM Peter Smith <smithpb2250@gmail.com> wrote:
Recently, I learned it is possible to say:
CREATE PUBLICATION pub ... WITH (publish_generated_columns);
This is equivalent to:
CREATE PUBLICATION pub ... WITH (publish_generated_columns = stored);~~~
Example:
postgres=# create publication pub for table t with
(publish_generated_columns=xxx);
ERROR: invalid value for publication parameter
"publish_generated_columns": "xxx"
DETAIL: Valid values are "none" and "stored".
postgres=# create publication pub for table t with (publish_generated_columns);
CREATE PUBLICATION
postgres=# \dRp pub
List of publications
Name | Owner | All tables | Inserts | Updates | Deletes | Truncates
| Generated columns | Via root
------+----------+------------+---------+---------+---------+-----------+-------------------+----------
pub | postgres | f | t | t | t | t
| stored | f
(1 row)~~~
However, I could not find this kind of shorthand specification (having
no assigned value) mentioned anywhere in the PG docs [1].OTOH, I did see this is being tested/commented in 'publication.sql'
[2], so perhaps that means it is deliberate?~~~
What is the verdict for this syntax -- ok or not?
I think we should give ERROR for this.
--
With Regards,
Amit Kapila.
Here is a v1 patch, where:
- now user gets ERROR if the 'publish_generated_columns' parameter is
specified without a value
- regression tests are updated
(docs are unchanged)
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v1-0001-CREATE-PUBLICATION-gencols-param-with-no-value.patchapplication/octet-stream; name=v1-0001-CREATE-PUBLICATION-gencols-param-with-no-value.patchDownload
From 6366bb009aa3912e1f2e780dce321cc51824e7cd Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Tue, 5 Aug 2025 09:55:58 +1000
Subject: [PATCH v1] CREATE PUBLICATION gencols param with no value
---
src/backend/commands/publicationcmds.c | 20 ++++++++++----------
src/test/regress/expected/publication.out | 10 ++++++----
src/test/regress/sql/publication.sql | 6 +++---
3 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 1bf7eaa..803c26a 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -2113,20 +2113,20 @@ AlterPublicationOwner_oid(Oid pubid, Oid newOwnerId)
static char
defGetGeneratedColsOption(DefElem *def)
{
- char *sval;
+ char *sval = "";
/*
- * If no parameter value given, assume "stored" is meant.
+ * A parameter value is required.
*/
- if (!def->arg)
- return PUBLISH_GENCOLS_STORED;
-
- sval = defGetString(def);
+ if (def->arg)
+ {
+ sval = defGetString(def);
- if (pg_strcasecmp(sval, "none") == 0)
- return PUBLISH_GENCOLS_NONE;
- if (pg_strcasecmp(sval, "stored") == 0)
- return PUBLISH_GENCOLS_STORED;
+ if (pg_strcasecmp(sval, "none") == 0)
+ return PUBLISH_GENCOLS_NONE;
+ if (pg_strcasecmp(sval, "stored") == 0)
+ return PUBLISH_GENCOLS_STORED;
+ }
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 1ec3fa3..bde8b19 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -36,6 +36,9 @@ LINE 1: ...pub_xxx WITH (publish_generated_columns = stored, publish_ge...
CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns = foo);
ERROR: invalid value for publication parameter "publish_generated_columns": "foo"
DETAIL: Valid values are "none" and "stored".
+CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns);
+ERROR: invalid value for publication parameter "publish_generated_columns": ""
+DETAIL: Valid values are "none" and "stored".
\dRp
List of publications
Name | Owner | All tables | Inserts | Updates | Deletes | Truncates | Generated columns | Via root
@@ -1844,8 +1847,7 @@ DROP SCHEMA sch1 cascade;
DROP SCHEMA sch2 cascade;
-- ======================================================
-- Test the 'publish_generated_columns' parameter with the following values:
--- 'stored', 'none', and the default (no value specified), which defaults to
--- 'stored'.
+-- 'stored', 'none'. When no parameter is specified it defaults to 'none'.
SET client_min_messages = 'ERROR';
CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_generated_columns = stored);
\dRp+ pub1
@@ -1863,12 +1865,12 @@ CREATE PUBLICATION pub2 FOR ALL TABLES WITH (publish_generated_columns = none);
regress_publication_user | t | t | t | t | t | none | f
(1 row)
-CREATE PUBLICATION pub3 FOR ALL TABLES WITH (publish_generated_columns);
+CREATE PUBLICATION pub3 FOR ALL TABLES;
\dRp+ pub3
Publication pub3
Owner | All tables | Inserts | Updates | Deletes | Truncates | Generated columns | Via root
--------------------------+------------+---------+---------+---------+-----------+-------------------+----------
- regress_publication_user | t | t | t | t | t | stored | f
+ regress_publication_user | t | t | t | t | t | none | f
(1 row)
DROP PUBLICATION pub1;
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 2585f08..e7f9b9a 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -26,6 +26,7 @@ CREATE PUBLICATION testpub_xxx WITH (publish = 'cluster, vacuum');
CREATE PUBLICATION testpub_xxx WITH (publish_via_partition_root = 'true', publish_via_partition_root = '0');
CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns = stored, publish_generated_columns = none);
CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns = foo);
+CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns);
\dRp
@@ -1183,14 +1184,13 @@ DROP SCHEMA sch2 cascade;
-- ======================================================
-- Test the 'publish_generated_columns' parameter with the following values:
--- 'stored', 'none', and the default (no value specified), which defaults to
--- 'stored'.
+-- 'stored', 'none'. When no parameter is specified it defaults to 'none'.
SET client_min_messages = 'ERROR';
CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_generated_columns = stored);
\dRp+ pub1
CREATE PUBLICATION pub2 FOR ALL TABLES WITH (publish_generated_columns = none);
\dRp+ pub2
-CREATE PUBLICATION pub3 FOR ALL TABLES WITH (publish_generated_columns);
+CREATE PUBLICATION pub3 FOR ALL TABLES;
\dRp+ pub3
DROP PUBLICATION pub1;
--
1.8.3.1
On Tue, 5 Aug 2025 at 05:35, Peter Smith <smithpb2250@gmail.com> wrote:
Here is a v1 patch, where:
- now user gets ERROR if the 'publish_generated_columns' parameter is
specified without a value
- regression tests are updated
Few comments:
1) Generally in other case we throw the following error "option
requires a parameter" for example:
postgres=# create subscription sub3 connection 'dbname=postgres
port=5432' publication pub1 with (synchronous_commit );
ERROR: synchronous_commit requires a parameter
postgres=# create publication pub1 for all tables with ( publish);
ERROR: publish requires a parameter
How about keeping it similar here too with below code:
/*
* A parameter value is required.
*/
if (def->arg == NULL)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s requires a parameter",
def->defname)));
2) This gets tested via other publication like testpub4, so this test
is not required:
-CREATE PUBLICATION pub3 FOR ALL TABLES WITH (publish_generated_columns);
+CREATE PUBLICATION pub3 FOR ALL TABLES;
\dRp+ pub3
Publication pub3
Owner | All tables | Inserts | Updates | Deletes
| Truncates | Generated columns | Via root
--------------------------+------------+---------+---------+---------+-----------+-------------------+----------
- regress_publication_user | t | t | t | t
| t | stored | f
+ regress_publication_user | t | t | t | t
| t | none | f
(1 row)This change is required for PG18 too, your patch applies for the PG18
branch also.
Regards,
Vignesh
On Tue, Aug 5, 2025 at 2:28 PM vignesh C <vignesh21@gmail.com> wrote:
On Tue, 5 Aug 2025 at 05:35, Peter Smith <smithpb2250@gmail.com> wrote:
Here is a v1 patch, where:
- now user gets ERROR if the 'publish_generated_columns' parameter is
specified without a value
- regression tests are updatedFew comments:
1) Generally in other case we throw the following error "option
requires a parameter" for example:
postgres=# create subscription sub3 connection 'dbname=postgres
port=5432' publication pub1 with (synchronous_commit );
ERROR: synchronous_commit requires a parameterpostgres=# create publication pub1 for all tables with ( publish);
ERROR: publish requires a parameterHow about keeping it similar here too with below code:
/*
* A parameter value is required.
*/
if (def->arg == NULL)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s requires a parameter",
def->defname)));
Hi Vignesh, thanks for the review.
I prefer to use a different message here for the following reasons:
a. I feel "%s requires a parameter" is inconsistent with the docs; "%s
requires a value" would make sense to me.
b. This patch case is not quite the same as other examples using "%s
requires a parameter", because 'publish_generated_columns' is an
*enum* parameter; therefore, we should output the valid values here
too, otherwise the message is not very helpful.
c. The already existing message of this function was very recently
improved by PeterE [1]https://github.com/postgres/postgres/commit/50fd428b2b9cb036c9c5982b56443d7e28119707#diff-2273de23c3b0087e9bc36a1a0a1eb844399dc67da882a2df778f574c8e0d9e93, so I prefer to keep the new message consistent
with that one.
2) This gets tested via other publication like testpub4, so this test is not required: -CREATE PUBLICATION pub3 FOR ALL TABLES WITH (publish_generated_columns); +CREATE PUBLICATION pub3 FOR ALL TABLES; \dRp+ pub3 Publication pub3 Owner | All tables | Inserts | Updates | Deletes | Truncates | Generated columns | Via root --------------------------+------------+---------+---------+---------+-----------+-------------------+---------- - regress_publication_user | t | t | t | t | t | stored | f + regress_publication_user | t | t | t | t | t | none | f (1 row)
OK. removed as suggested.
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v2-0001-CREATE-PUBLICATION-gencols-param-with-no-value.patchapplication/octet-stream; name=v2-0001-CREATE-PUBLICATION-gencols-param-with-no-value.patchDownload
From 3dfb0baabefcf5b0ea05c0ac4fffa7421b0f34f5 Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Tue, 5 Aug 2025 16:14:25 +1000
Subject: [PATCH v2] CREATE PUBLICATION gencols param with no value
---
src/backend/commands/publicationcmds.c | 8 ++++----
src/test/regress/expected/publication.out | 15 ++++-----------
src/test/regress/sql/publication.sql | 7 ++-----
3 files changed, 10 insertions(+), 20 deletions(-)
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 1bf7eaa..97d4474 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -2115,11 +2115,11 @@ defGetGeneratedColsOption(DefElem *def)
{
char *sval;
- /*
- * If no parameter value given, assume "stored" is meant.
- */
if (!def->arg)
- return PUBLISH_GENCOLS_STORED;
+ ereport(ERROR,
+ errcode(ERRCODE_SYNTAX_ERROR),
+ errmsg("missing value for publication parameter \"%s\"", def->defname),
+ errdetail("Valid values are \"%s\" and \"%s\".", "none", "stored"));
sval = defGetString(def);
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 1ec3fa3..42e6e74 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -36,6 +36,9 @@ LINE 1: ...pub_xxx WITH (publish_generated_columns = stored, publish_ge...
CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns = foo);
ERROR: invalid value for publication parameter "publish_generated_columns": "foo"
DETAIL: Valid values are "none" and "stored".
+CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns);
+ERROR: missing value for publication parameter "publish_generated_columns"
+DETAIL: Valid values are "none" and "stored".
\dRp
List of publications
Name | Owner | All tables | Inserts | Updates | Deletes | Truncates | Generated columns | Via root
@@ -1844,8 +1847,7 @@ DROP SCHEMA sch1 cascade;
DROP SCHEMA sch2 cascade;
-- ======================================================
-- Test the 'publish_generated_columns' parameter with the following values:
--- 'stored', 'none', and the default (no value specified), which defaults to
--- 'stored'.
+-- 'stored', 'none'.
SET client_min_messages = 'ERROR';
CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_generated_columns = stored);
\dRp+ pub1
@@ -1863,17 +1865,8 @@ CREATE PUBLICATION pub2 FOR ALL TABLES WITH (publish_generated_columns = none);
regress_publication_user | t | t | t | t | t | none | f
(1 row)
-CREATE PUBLICATION pub3 FOR ALL TABLES WITH (publish_generated_columns);
-\dRp+ pub3
- Publication pub3
- Owner | All tables | Inserts | Updates | Deletes | Truncates | Generated columns | Via root
---------------------------+------------+---------+---------+---------+-----------+-------------------+----------
- regress_publication_user | t | t | t | t | t | stored | f
-(1 row)
-
DROP PUBLICATION pub1;
DROP PUBLICATION pub2;
-DROP PUBLICATION pub3;
-- Test the 'publish_generated_columns' parameter as 'none' and 'stored' for
-- different scenarios with/without generated columns in column lists.
CREATE TABLE gencols (a int, gen1 int GENERATED ALWAYS AS (a * 2) STORED);
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 2585f08..deddf0d 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -26,6 +26,7 @@ CREATE PUBLICATION testpub_xxx WITH (publish = 'cluster, vacuum');
CREATE PUBLICATION testpub_xxx WITH (publish_via_partition_root = 'true', publish_via_partition_root = '0');
CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns = stored, publish_generated_columns = none);
CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns = foo);
+CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns);
\dRp
@@ -1183,19 +1184,15 @@ DROP SCHEMA sch2 cascade;
-- ======================================================
-- Test the 'publish_generated_columns' parameter with the following values:
--- 'stored', 'none', and the default (no value specified), which defaults to
--- 'stored'.
+-- 'stored', 'none'.
SET client_min_messages = 'ERROR';
CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_generated_columns = stored);
\dRp+ pub1
CREATE PUBLICATION pub2 FOR ALL TABLES WITH (publish_generated_columns = none);
\dRp+ pub2
-CREATE PUBLICATION pub3 FOR ALL TABLES WITH (publish_generated_columns);
-\dRp+ pub3
DROP PUBLICATION pub1;
DROP PUBLICATION pub2;
-DROP PUBLICATION pub3;
-- Test the 'publish_generated_columns' parameter as 'none' and 'stored' for
-- different scenarios with/without generated columns in column lists.
--
1.8.3.1
On Tue, Aug 5, 2025 at 11:57 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Tue, Aug 5, 2025 at 2:28 PM vignesh C <vignesh21@gmail.com> wrote:
On Tue, 5 Aug 2025 at 05:35, Peter Smith <smithpb2250@gmail.com> wrote:
Here is a v1 patch, where:
- now user gets ERROR if the 'publish_generated_columns' parameter is
specified without a value
- regression tests are updatedFew comments:
1) Generally in other case we throw the following error "option
requires a parameter" for example:
postgres=# create subscription sub3 connection 'dbname=postgres
port=5432' publication pub1 with (synchronous_commit );
ERROR: synchronous_commit requires a parameterpostgres=# create publication pub1 for all tables with ( publish);
ERROR: publish requires a parameterHow about keeping it similar here too with below code:
/*
* A parameter value is required.
*/
if (def->arg == NULL)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s requires a parameter",
def->defname)));Hi Vignesh, thanks for the review.
I prefer to use a different message here for the following reasons:
a. I feel "%s requires a parameter" is inconsistent with the docs; "%s
requires a value" would make sense to me.b. This patch case is not quite the same as other examples using "%s
requires a parameter", because 'publish_generated_columns' is an
*enum* parameter; therefore, we should output the valid values here
too, otherwise the message is not very helpful.
+CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns);
+ERROR: invalid value for publication parameter "publish_generated_columns": ""
+DETAIL: Valid values are "none" and "stored".I find this message clearer and helpful for users. So, +1 for retaining it.
--
With Regards,
Amit Kapila.
On Tue, Aug 5, 2025 at 4:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 5, 2025 at 11:57 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Tue, Aug 5, 2025 at 2:28 PM vignesh C <vignesh21@gmail.com> wrote:
On Tue, 5 Aug 2025 at 05:35, Peter Smith <smithpb2250@gmail.com> wrote:
Here is a v1 patch, where:
- now user gets ERROR if the 'publish_generated_columns' parameter is
specified without a value
- regression tests are updatedFew comments:
1) Generally in other case we throw the following error "option
requires a parameter" for example:
postgres=# create subscription sub3 connection 'dbname=postgres
port=5432' publication pub1 with (synchronous_commit );
ERROR: synchronous_commit requires a parameterpostgres=# create publication pub1 for all tables with ( publish);
ERROR: publish requires a parameterHow about keeping it similar here too with below code:
/*
* A parameter value is required.
*/
if (def->arg == NULL)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s requires a parameter",
def->defname)));Hi Vignesh, thanks for the review.
I prefer to use a different message here for the following reasons:
a. I feel "%s requires a parameter" is inconsistent with the docs; "%s
requires a value" would make sense to me.b. This patch case is not quite the same as other examples using "%s
requires a parameter", because 'publish_generated_columns' is an
*enum* parameter; therefore, we should output the valid values here
too, otherwise the message is not very helpful.+CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns); +ERROR: invalid value for publication parameter "publish_generated_columns": "" +DETAIL: Valid values are "none" and "stored".I find this message clearer and helpful for users. So, +1 for retaining it.
Sure. Please find the v3 patch.
Everything is the same as the original v1, except that the redundant
test case was removed per Vignesh's review..
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Attachments:
v3-0001-CREATE-PUBLICATION-gencols-param-with-no-value.patchapplication/octet-stream; name=v3-0001-CREATE-PUBLICATION-gencols-param-with-no-value.patchDownload
From 583490697652365ab17444c312daecc92fe6799a Mon Sep 17 00:00:00 2001
From: Peter Smith <peter.b.smith@fujitsu.com>
Date: Tue, 5 Aug 2025 17:16:11 +1000
Subject: [PATCH v3] CREATE PUBLICATION gencols param with no value
---
src/backend/commands/publicationcmds.c | 20 ++++++++++----------
src/test/regress/expected/publication.out | 15 ++++-----------
src/test/regress/sql/publication.sql | 7 ++-----
3 files changed, 16 insertions(+), 26 deletions(-)
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 1bf7eaa..803c26a 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -2113,20 +2113,20 @@ AlterPublicationOwner_oid(Oid pubid, Oid newOwnerId)
static char
defGetGeneratedColsOption(DefElem *def)
{
- char *sval;
+ char *sval = "";
/*
- * If no parameter value given, assume "stored" is meant.
+ * A parameter value is required.
*/
- if (!def->arg)
- return PUBLISH_GENCOLS_STORED;
-
- sval = defGetString(def);
+ if (def->arg)
+ {
+ sval = defGetString(def);
- if (pg_strcasecmp(sval, "none") == 0)
- return PUBLISH_GENCOLS_NONE;
- if (pg_strcasecmp(sval, "stored") == 0)
- return PUBLISH_GENCOLS_STORED;
+ if (pg_strcasecmp(sval, "none") == 0)
+ return PUBLISH_GENCOLS_NONE;
+ if (pg_strcasecmp(sval, "stored") == 0)
+ return PUBLISH_GENCOLS_STORED;
+ }
ereport(ERROR,
errcode(ERRCODE_SYNTAX_ERROR),
diff --git a/src/test/regress/expected/publication.out b/src/test/regress/expected/publication.out
index 1ec3fa3..5326805 100644
--- a/src/test/regress/expected/publication.out
+++ b/src/test/regress/expected/publication.out
@@ -36,6 +36,9 @@ LINE 1: ...pub_xxx WITH (publish_generated_columns = stored, publish_ge...
CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns = foo);
ERROR: invalid value for publication parameter "publish_generated_columns": "foo"
DETAIL: Valid values are "none" and "stored".
+CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns);
+ERROR: invalid value for publication parameter "publish_generated_columns": ""
+DETAIL: Valid values are "none" and "stored".
\dRp
List of publications
Name | Owner | All tables | Inserts | Updates | Deletes | Truncates | Generated columns | Via root
@@ -1844,8 +1847,7 @@ DROP SCHEMA sch1 cascade;
DROP SCHEMA sch2 cascade;
-- ======================================================
-- Test the 'publish_generated_columns' parameter with the following values:
--- 'stored', 'none', and the default (no value specified), which defaults to
--- 'stored'.
+-- 'stored', 'none'.
SET client_min_messages = 'ERROR';
CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_generated_columns = stored);
\dRp+ pub1
@@ -1863,17 +1865,8 @@ CREATE PUBLICATION pub2 FOR ALL TABLES WITH (publish_generated_columns = none);
regress_publication_user | t | t | t | t | t | none | f
(1 row)
-CREATE PUBLICATION pub3 FOR ALL TABLES WITH (publish_generated_columns);
-\dRp+ pub3
- Publication pub3
- Owner | All tables | Inserts | Updates | Deletes | Truncates | Generated columns | Via root
---------------------------+------------+---------+---------+---------+-----------+-------------------+----------
- regress_publication_user | t | t | t | t | t | stored | f
-(1 row)
-
DROP PUBLICATION pub1;
DROP PUBLICATION pub2;
-DROP PUBLICATION pub3;
-- Test the 'publish_generated_columns' parameter as 'none' and 'stored' for
-- different scenarios with/without generated columns in column lists.
CREATE TABLE gencols (a int, gen1 int GENERATED ALWAYS AS (a * 2) STORED);
diff --git a/src/test/regress/sql/publication.sql b/src/test/regress/sql/publication.sql
index 2585f08..deddf0d 100644
--- a/src/test/regress/sql/publication.sql
+++ b/src/test/regress/sql/publication.sql
@@ -26,6 +26,7 @@ CREATE PUBLICATION testpub_xxx WITH (publish = 'cluster, vacuum');
CREATE PUBLICATION testpub_xxx WITH (publish_via_partition_root = 'true', publish_via_partition_root = '0');
CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns = stored, publish_generated_columns = none);
CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns = foo);
+CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns);
\dRp
@@ -1183,19 +1184,15 @@ DROP SCHEMA sch2 cascade;
-- ======================================================
-- Test the 'publish_generated_columns' parameter with the following values:
--- 'stored', 'none', and the default (no value specified), which defaults to
--- 'stored'.
+-- 'stored', 'none'.
SET client_min_messages = 'ERROR';
CREATE PUBLICATION pub1 FOR ALL TABLES WITH (publish_generated_columns = stored);
\dRp+ pub1
CREATE PUBLICATION pub2 FOR ALL TABLES WITH (publish_generated_columns = none);
\dRp+ pub2
-CREATE PUBLICATION pub3 FOR ALL TABLES WITH (publish_generated_columns);
-\dRp+ pub3
DROP PUBLICATION pub1;
DROP PUBLICATION pub2;
-DROP PUBLICATION pub3;
-- Test the 'publish_generated_columns' parameter as 'none' and 'stored' for
-- different scenarios with/without generated columns in column lists.
--
1.8.3.1
On Tue, 5 Aug 2025 at 12:52, Peter Smith <smithpb2250@gmail.com> wrote:
On Tue, Aug 5, 2025 at 4:43 PM Amit Kapila <amit.kapila16@gmail.com> wrote:
On Tue, Aug 5, 2025 at 11:57 AM Peter Smith <smithpb2250@gmail.com> wrote:
On Tue, Aug 5, 2025 at 2:28 PM vignesh C <vignesh21@gmail.com> wrote:
On Tue, 5 Aug 2025 at 05:35, Peter Smith <smithpb2250@gmail.com> wrote:
Here is a v1 patch, where:
- now user gets ERROR if the 'publish_generated_columns' parameter is
specified without a value
- regression tests are updatedFew comments:
1) Generally in other case we throw the following error "option
requires a parameter" for example:
postgres=# create subscription sub3 connection 'dbname=postgres
port=5432' publication pub1 with (synchronous_commit );
ERROR: synchronous_commit requires a parameterpostgres=# create publication pub1 for all tables with ( publish);
ERROR: publish requires a parameterHow about keeping it similar here too with below code:
/*
* A parameter value is required.
*/
if (def->arg == NULL)
ereport(ERROR,
(errcode(ERRCODE_SYNTAX_ERROR),
errmsg("%s requires a parameter",
def->defname)));Hi Vignesh, thanks for the review.
I prefer to use a different message here for the following reasons:
a. I feel "%s requires a parameter" is inconsistent with the docs; "%s
requires a value" would make sense to me.b. This patch case is not quite the same as other examples using "%s
requires a parameter", because 'publish_generated_columns' is an
*enum* parameter; therefore, we should output the valid values here
too, otherwise the message is not very helpful.+CREATE PUBLICATION testpub_xxx WITH (publish_generated_columns); +ERROR: invalid value for publication parameter "publish_generated_columns": "" +DETAIL: Valid values are "none" and "stored".I find this message clearer and helpful for users. So, +1 for retaining it.
Sure. Please find the v3 patch.
Everything is the same as the original v1, except that the redundant
test case was removed per Vignesh's review..
Thanks, this version looks good.
Regards,
Vignesh