On Tue, 5 Aug 2025 at 12:52, Peter Smith <[email protected]> wrote: > > On Tue, Aug 5, 2025 at 4:43 PM Amit Kapila <[email protected]> wrote: > > > > On Tue, Aug 5, 2025 at 11:57 AM Peter Smith <[email protected]> wrote: > > > > > > On Tue, Aug 5, 2025 at 2:28 PM vignesh C <[email protected]> wrote: > > > > > > > > On Tue, 5 Aug 2025 at 05:35, Peter Smith <[email protected]> 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
