Horton

Thread

Commits

  1. Throw ERROR when publish_generated_columns is specified without a value.

  2. Message style improvements

  1. CREATE PUBLICATION with 'publish_generated_columns' parameter specified but unassigned

    Peter Smith <smithpb2250@gmail.com> — 2025-08-04T06:23:36Z

    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].

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?

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

  2. Re: CREATE PUBLICATION with 'publish_generated_columns' parameter specified but unassigned

    David G. Johnston <david.g.johnston@gmail.com> — 2025-08-04T06:37:58Z

    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.

  3. Re: CREATE PUBLICATION with 'publish_generated_columns' parameter specified but unassigned

    Peter Smith <smithpb2250@gmail.com> — 2025-08-04T08:44:45Z

    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

  4. Re: CREATE PUBLICATION with 'publish_generated_columns' parameter specified but unassigned

    Amit Kapila <amit.kapila16@gmail.com> — 2025-08-04T09:01:03Z

    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.

  5. Re: CREATE PUBLICATION with 'publish_generated_columns' parameter specified but unassigned

    Peter Smith <smithpb2250@gmail.com> — 2025-08-05T00:05:15Z

    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

  6. Re: CREATE PUBLICATION with 'publish_generated_columns' parameter specified but unassigned

    vignesh C <vignesh21@gmail.com> — 2025-08-05T04:28:33Z

    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

  7. Re: CREATE PUBLICATION with 'publish_generated_columns' parameter specified but unassigned

    Peter Smith <smithpb2250@gmail.com> — 2025-08-05T06:26:34Z

    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 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)));
>

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], 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.

======
[1] https://github.com/postgres/postgres/commit/50fd428b2b9cb036c9c5982b56443d7e28119707#diff-2273de23c3b0087e9bc36a1a0a1eb844399dc67da882a2df778f574c8e0d9e93

Kind Regards,
Peter Smith.
Fujitsu Australia

  8. Re: CREATE PUBLICATION with 'publish_generated_columns' parameter specified but unassigned

    Amit Kapila <amit.kapila16@gmail.com> — 2025-08-05T06:43:14Z

    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 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)));
> >
>
> 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.

  9. Re: CREATE PUBLICATION with 'publish_generated_columns' parameter specified but unassigned

    Peter Smith <smithpb2250@gmail.com> — 2025-08-05T07:22:06Z

    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 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)));
> > >
> >
> > 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

  10. Re: CREATE PUBLICATION with 'publish_generated_columns' parameter specified but unassigned

    vignesh C <vignesh21@gmail.com> — 2025-08-05T08:52:34Z

    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 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)));
> > > >
> > >
> > > 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