Обсуждение: Custom variables and flags, again

The auto-explain patch needs a way to set the GUC "flags" for a custom
GUC variable; a capability that was unaccountably left out of the
original DefineCustomXXXVariable APIs.  The original version of the patch
dealt with this by changing those APIs, which I complained about here:
http://archives.postgresql.org/pgsql-hackers/2008-09/msg00168.php

The new version gets around this by leaving those functions alone and
exporting a new function void DefineCustomVariable(enum config_type type, const void *variable);
to which you are supposed to pass an entire struct config_whatever.

I can't say that I find this an improvement.  Yeah, it avoids breaking
existing code, but the idea that add-on modules should hard-code the
exact contents of the GUC structs fills me with horror.  What happens
when we add some field or other to those structs?  I'll tell you what
happens: those modules break, nastily and probably silently, because
no C compiler I know of is very picky about the contents of a struct
initializer.  So I think this is just trading short-term pain for a
larger amount of long-term pain.

On the whole, I think we'd be better off to just add more arguments
to the DefineCustomXXXVariable functions, as in the original patch
(and while we're at it, fix the problem of where the boot_val comes
from, as suggested in the above-referenced message).  At least that will
result in obvious and easily-fixable breakage in add-on modules that use
these functions.

Comments?
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> wrote:

>   void DefineCustomVariable(enum config_type type, const void *variable);
> What happens
> when we add some field or other to those structs?

I considered the case and chose the new interface because it behaves
*well* in the case. We can freely add new fields at the end of structs
(config_generic and config_<type>) as long as zero means 'default' in
the new fields. Unassigned fields in struct variables are filled with
zero in C.

There are problems when we modify the middle fields in those
structs, but it means modification of existing arguments in
DefineCustomXXXVariable(); The same problems occur in both
implementations.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What happens
>> when we add some field or other to those structs?

> There are problems when we modify the middle fields in those
> structs, but it means modification of existing arguments in
> DefineCustomXXXVariable(); The same problems occur in both
> implementations.

No, they are not the same problems.  You can rely on the C compiler
to complain if you aren't passing enough arguments to a function.
You can't rely on it to complain if your struct constant is putting
values into the wrong fields.

Perhaps more to the point, guc_tables.h is a file that we don't even
want the majority of the backend including.  Why would we think it's
a good idea to make that part of the public API to external modules?
        regards, tom lane

Tom Lane <tgl@sss.pgh.pa.us> wrote:

> Perhaps more to the point, guc_tables.h is a file that we don't even
> want the majority of the backend including.  Why would we think it's
> a good idea to make that part of the public API to external modules?

Hmmm... do we need to move config_group enumerations to guc.h from
guc_tables.h ? Or, we cannot expose the group field and always use
CUSTOM_OPTIONS for custom variables.

Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center

ITAGAKI Takahiro <itagaki.takahiro@oss.ntt.co.jp> writes:
> Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Perhaps more to the point, guc_tables.h is a file that we don't even
>> want the majority of the backend including.  Why would we think it's
>> a good idea to make that part of the public API to external modules?

> Hmmm... do we need to move config_group enumerations to guc.h from
> guc_tables.h ? Or, we cannot expose the group field and always use
> CUSTOM_OPTIONS for custom variables.

My inclination is not to expose that, and just use CUSTOM_OPTIONS
always.  As was noted earlier, the config_group enumeration seems
really ad-hoc and subject to change, so keeping external modules away
from it seems the best thing to me.

We do need to move some of the GUC flags #defines (not sure about all
of them...) into guc.h, if we want to take the position that external
modules shouldn't import guc_tables.h.
        regards, tom lane

> On the whole, I think we'd be better off to just add more arguments
> to the DefineCustomXXXVariable functions, as in the original patch
> (and while we're at it, fix the problem of where the boot_val comes
> from, as suggested in the above-referenced message).  At least that will
> result in obvious and easily-fixable breakage in add-on modules that use
> these functions.

+1.  Obvious breakage is much better than subtle breakage.

...Robert