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
