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

On Tue, Aug 5, 2025 at 4:43 PM Amit Kapila <amit(dot)kapila16(at)gmail(dot)com> wrote:
>
> On Tue, Aug 5, 2025 at 11:57 AM Peter Smith <smithpb2250(at)gmail(dot)com> wrote:
> >
> > On Tue, Aug 5, 2025 at 2:28 PM vignesh C <vignesh21(at)gmail(dot)com> wrote:
> > >
> > > On Tue, 5 Aug 2025 at 05:35, Peter Smith <smithpb2250(at)gmail(dot)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