Date: Wed, 5 Jan 2022 23:36:42 -0600
From: Justin Pryzby <pryzby@telsasoft.com>
To: Michael Paquier <michael@paquier.xyz>
Cc: Kyotaro Horiguchi <horikyota.ntt@gmail.com>,
	peter.eisentraut@enterprisedb.com,
	pgsql-hackers@lists.postgresql.org, bruce@momjian.us,
	tgl@sss.pgh.pa.us
Subject: Re: GUC flags
Message-ID: <20220106053641.GW14051@telsasoft.com>
References: <YbG7sp+2YEYgbY0D@paquier.xyz>
 <20211209155322.GG17618@telsasoft.com>
 <20211229023240.GM24477@telsasoft.com>
 <20220105.114757.369416916044334409.horikyota.ntt@gmail.com>
 <20220105030648.GE14051@telsasoft.com>
 <YdUp118ekFgGiixT@paquier.xyz>
 <20220105235517.GR14051@telsasoft.com>
 <YdZ7zCymonPV4//X@paquier.xyz>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <YdZ7zCymonPV4//X@paquier.xyz>
User-Agent: Mutt/1.9.4 (2018-02-28)
Archived-At: 
 <https://www.postgresql.org/message-id/20220106053641.GW14051%40telsasoft.com>
Precedence: bulk

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