On Tue, Mar 20, 2018 at 01:27:35PM -0400, Tom Lane wrote: > 1. Only GUC_LIST_QUOTE variables need to be special-cased. It works > fine to treat plain LIST variables (e.g. datestyle) with the regular > code path. This is good because there are so few of them.
Check. > 2. We should make an effort to minimize the number of places that know > explicitly which variables are GUC_LIST_QUOTE. In the backend, the > right thing to do is ask guc.c. In pg_dump, there's no convenient > way to ask the backend (and certainly nothing that would work against > old servers), but we should at least make sure there's only one place > to fix not two. The only way I can think about for that would be to provide this information in pg_settings using a text[] array or such which lists all the GUC flags of a parameter. That's a hell lot of infrastructure though which can be easily replaced with the maintenance of a small hardcoded parameter list. > 3. We need to prevent extensions from trying to create GUC_LIST_QUOTE > variables, because those are not going to work correctly if they're > set before the extension is loaded, nor is there any hope of pg_dump > dumping them correctly. This really depends on the elements of the list involved here, so pg_dump may be able to dump them correctly, though not reliably as that would be fully application-dependent. At the end I think that I am on board with what you are proposing here. > The attached 0001 patch does the first two things, and could be > back-patched. The 0002 patch, which I'd only propose for HEAD, > is one way we could address point 3. A different way would be > to throw a WARNING rather than ERROR and then just mask off the > problematic bit. Or we could decide that either of these cures > is worse than the disease, but I don't really agree with that. Looks roughly sane to me. + if (flags & GUC_LIST_QUOTE) + elog(FATAL, "extensions cannot define GUC_LIST_QUOTE variables"); This would be better as an ereport with ERRCODE_FEATURE_NOT_SUPPORTED I think. An ERROR is better in my opinion. - if (pg_strcasecmp(configitem, "DateStyle") == 0 - || pg_strcasecmp(configitem, "search_path") == 0) + if (GetConfigOptionFlags(configitem, true) & GUC_LIST_QUOTE) For boolean-based comparisons, I would recommend using a comparison with zero. + record = find_option(name, false, WARNING); + if (record == NULL) LOG instead of WARNING? -- Michael
signature.asc
Description: PGP signature
