On Mon, 19 Jan 2026 at 18:37, Peter Eisentraut <[email protected]> wrote: > > On 10.01.26 07:16, Paul A Jungwirth wrote: > > We would need to document these columns. > > Done that. > > > The C code uses `mltrng` a lot. Do we want to use that here? I don't > > see it in the catalog yet, but it seems clearer than `rngm`. I guess > > we have to start with `rng` though. We have `rngmultitypid`, so maybe > > `rngmulticonstr0`? Okay I understand why you went with `rngm`. > > I tuned the naming again in the new patch. I changed "constr" to > "construct" because "constr" read too much like "constraint" to me. I > also did a bit of "mtlrng". I think it's a bit more consistent and less > ambiguous now. > > > It's tempting to use two oidvectors, one for range constructors and > > another for multirange, with the 0-arg constructor in position 0, > > 1-arg in position 1, etc. We could use InvalidOid to say there is no > > such constructor. So we would have rngconstr of `{0,0,123,456}` and > > mltrngconstr of `{123,456,789}`. But is it better to avoid varlena > > columns if we can? > > I don't think oidvectors would be appropriate here. These are for when > you have a group of values that you need together, like for function > arguments. But here we want to access them separately. And it would > create a lot of notational and a bit of storage overhead. > > I had in the previous patch used some arrays as arguments in the > internal functions, but in the second patch I'm also getting rid of that > because it's uselessly inconsistent. > > > ``` > > diff --git a/src/include/catalog/pg_range.h b/src/include/catalog/pg_range.h > > index 5b4f4615905..ad4d1e9187f 100644 > > --- a/src/include/catalog/pg_range.h > > +++ b/src/include/catalog/pg_range.h > > @@ -43,6 +43,15 @@ CATALOG(pg_range,3541,RangeRelationId) > > /* subtype's btree opclass */ > > Oid rngsubopc BKI_LOOKUP(pg_opclass); > > > > + /* range constructor functions */ > > + regproc rngconstr2 BKI_LOOKUP(pg_proc); > > + regproc rngconstr3 BKI_LOOKUP(pg_proc); > > + > > + /* multirange constructor functions */ > > + regproc rngmconstr0 BKI_LOOKUP(pg_proc); > > + regproc rngmconstr1 BKI_LOOKUP(pg_proc); > > + regproc rngmconstr2 BKI_LOOKUP(pg_proc); > > + > > /* canonicalize range, or 0 */ > > regproc rngcanonical BKI_LOOKUP_OPT(pg_proc); > > ``` > > > > Is there a reason you're adding them in the middle of the struct? It > > doesn't help with packing. > > Well, initially I had done that so that the edits to pg_range.dat are > easier. But I think this order makes some sense, because it has the > mandatory data first and then the optional data later. But it doesn't > matter much either way. > > > This needs some kind of pg_upgrade support I assume? It will have to > > work for user-defined rangetypes too. > > No, I don't think there needs to be pg_upgrade support. Existing range > types are dumped as CREATE TYPE ... RANGE commands, and when those get > restored it will create the new catalog entries.
Hi! I have looked into v2. This patch looks good. Making explicit links in pg_catalog seems to be more cve-proof to me. Using Paul's approach (get_typname_and_namespace) is not only fragile, it is a recipe for CVE if any mistake is made, is it? I mean, matching something by name is vulnerable for search-path-based CVE (again, not saying this is the case in Paul patch). I think patch tests are good. Also, I don't think we need to mention any "upcoming patches" in the commit message - this change has its own value. One stupid question from me: should we add ```` t.typanalyze!='range_typanalyze'::regproc or t.typinput != 'range_in'::regproc or t.typoutput != 'range_out'::regproc or t.typreceive != 'range_recv'::regproc or typsend != 'range_send'::regproc; ```` In type sanity sql check? In my understanding, this condition (t.typanalyze == 'range_typanalyze'::regproc and ....) is required for built-in range types, and for user-defined seems to also be true. -- Best regards, Kirill Reshke
