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

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