On Thu, Jan 06, 2022 at 02:19:08PM +0900, Michael Paquier wrote:
> On Wed, Jan 05, 2022 at 05:55:17PM -0600, Justin Pryzby wrote:
> > pg_settings is currently defined with "SELECT *". Is it fine to enumerate a
> > list of columns instead ?
>
> I'd like to think that this is a better practice when it comes
> documenting the columns, but I don't see an actual need for this extra
> complication here.
The reason is to avoid showing the flags in the pg_settings view, which should
not be bloated just so we can retire check_guc.
> > + initStringInfo(&ret);
> > + appendStringInfoChar(&ret, '{');
> > +
> > + if (flags & GUC_NO_SHOW_ALL)
> > + appendStringInfo(&ret, "NO_SHOW_ALL,");
> > + if (flags & GUC_NO_RESET_ALL)
> > + appendStringInfo(&ret, "NO_RESET_ALL,");
> > + if (flags & GUC_NOT_IN_SAMPLE)
> > + appendStringInfo(&ret, "NOT_IN_SAMPLE,");
> > + if (flags & GUC_EXPLAIN)
> > + appendStringInfo(&ret, "EXPLAIN,");
> > + if (flags & GUC_RUNTIME_COMPUTED)
> > + appendStringInfo(&ret, "RUNTIME_COMPUTED,");
> > +
> > + /* Remove trailing comma, if any */
> > + if (ret.len > 1)
> > + ret.data[--ret.len] = '\0';
>
> The way of building the text array is incorrect here. See
> heap_tuple_infomask_flags() in pageinspect as an example with all the
> HEAP_* flags. I think that you should allocate an array of Datums,
> use CStringGetTextDatum() to assign each array element, wrapping the
> whole with construct_array() to build the final value for the
> parameter tuple.
I actually did it that way last night ... however GetConfigOptionByNum() is
expecting it to return a text string, not an array.
--
Justin
