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+19-14
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+10-16
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+16-22
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