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

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