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

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.

--
With Regards,
Amit Kapila.