PGC_S_ARGV vs pg_file_settings and pg_settings

Hello hackers,

I am investigating a false error reported in pg_file_settings when some
parameters are set in PGC_S_ARGV. In the following example, shared_buffers
appears twice in postgresql.conf and has been set on command line using
"--shared_buffers=512MB":

$ cat $PGDATA/postmaster.opts
…/postgres "-D" "…/20argv" "--shared_buffers=512MB"

postgres=# SHOW shared_buffers;
shared_buffers
----------------
512MB

postgres=# SELECT name, setting, applied, error
FROM pg_file_settings
WHERE name = 'shared_buffers';
name | setting | applied | error
----------------+---------+---------+------------------------------
shared_buffers | 128MB | f | Ø
shared_buffers | 256MB | f | setting could not be applied

There's no error forbidding to set "shared_buffers" here, just normal
overriding. In consequence, I would expect the second line not to expose an
error.

The documentation should probably describe this as an exception when
applied=false with no error, like it does for duplicated values. See in
system-views.sgml:

[…] Another way that
an entry might have <structfield>applied</structfield> = false is that it is
overridden by a later entry for the same parameter name; this case is not
considered an error so nothing appears in
the <structfield>error</structfield> field.

When looking at the code, there's something I don't understand in
ProcessConfigFileInternal(). It seems the code doesn't follow the comment here:

scres = set_config_option(item->name, item->value,
context, PGC_S_FILE,
GUC_ACTION_SET, applySettings, 0, false);
if (scres > 0) […]
else if (scres == 0) […]
else
{
/* no error, but variable's active value was not changed */
item->applied = true;
}

Why the GUC is flagged "applied" if set_config_option() returns -1? Header
comment of this function describe:

/* […]
* -1: no error detected, but the value was not applied, either
* because changeVal is false or there is some overriding setting.

Please, find in attachment a patch fixing the later incoherence, and returning
-1 instead of 0 if a GUC in config file is overridden by any mean when
prohibitValueChange applies.

Note that as a side effect, it also change the behavior of
pg_settings.pending_restart in such a case. Without the patch:

postgres=# SELECT name, setting, source, sourcefile, pending_restart
FROM pg_settings
WHERE name = 'shared_buffers';
name | setting | source | sourcefile | pending_restart
----------------+---------+--------------+------------+-----------------
shared_buffers | 65536 | command line | Ø | t

With the patch:

postgres=# SELECT name, setting, applied, error
FROM pg_file_settings
WHERE name = 'shared_buffers';
name | setting | applied | error
----------------+---------+---------+-------
shared_buffers | 128MB | f | Ø
shared_buffers | 256MB | f | Ø

postgres=# SELECT name, setting, source, sourcefile, pending_restart
FROM pg_settings
WHERE name = 'shared_buffers';
name | setting | source | sourcefile | pending_restart
----------------+---------+--------------+------------+-----------------
shared_buffers | 65536 | command line | Ø | f

I'm a bit confused about what pending_restart should really expose in such
situation, but I feel like it should be 'f'… maybe with a comment somewhere.

Regards,