On Tue, Mar 17, 2026 at 3:50 PM Viktor Holmberg <[email protected]> wrote:
> On 17 Mar 2026 at 09:06 +0100, jian he <[email protected]>, > wrote: > > This attached is more bullet-proof. > > My previous question was more of syntax question, and it has been > addressed in the latest patch. > > Questions/ comments: > > The commitfest entry has two other reviewers already, but I haven’t seen > any emails from them? If you are still reviewing Yogesh or Aditya, maybe > let us know, or else might be best to remove them from the entry so it’s > visible you’re waiting for reviewers. > > ----------------------- > > + or an unconstrained domain over the new type, or domain over new type > has no > + volatile constraint, a table rewrite is not needed. > + However, indexes will still be rebuilt unless the system > > Why do we need to care about volatility in this patch? Wouldn’t things > work with skipping the rewrite then checking the volatile checks? A rewrite > doesn’t make things more deterministic right? > > ------------------------ > > +DROP DOMAIN domain1, domain2, domain3, domain4, domain5; > +ERROR: cannot drop desired object(s) because other objects depend on them > +DETAIL: type domain6 depends on type domain2[] > +HINT: Use DROP ... CASCADE to drop the dependent objects too. > > +ERROR: cannot drop schema fast_default because other objects depend on it > +DETAIL: type domain1 depends on schema fast_default > +type domain2 depends on schema fast_default > +type domain3 depends on schema fast_default > +type domain4 depends on schema fast_default > +type domain5 depends on schema fast_default > +type domain6 depends on schema fast_default > +HINT: Use DROP ... CASCADE to drop the dependent objects too. > > Unnecessary noise in the test output? > > ----------------------- > > Add a test like this: > > CREATE DOMAIN domain1 AS INT CHECK(VALUE > 1) NOT NULL; > CREATE TABLE t_const_using(a INT); > INSERT INTO t_const_using VALUES(-2); > ALTER TABLE t_const_using ALTER COLUMN a SET DATA TYPE domain1 USING 5; > SELECT a FROM t_const_using; -- should be 5 after rewrite, not -2 > a > ---- > -2 > > So something is wrong with constant values. > > ----------------------- > > Minor spelling/grammar: > - Typo: igrnore → ignore (line ~6552) > - Grammar issues in comments and the commit message ("We can just using > table scan", "ensure exists content is compatibility", "the new domain type > all constraint are non-volatile"). > > ----------------------- > > This is very much a non-exhaustive review as I in all honestly understand > maybe like 20% of this code. But at least it’s a start. > Hello! Apologies for the late timing but Yogesh and I still plan on reviewing this patch. This is one of my first patch reviews actually as part of the patch review workshop! With that being said I would love to share some of my initial thoughts, and to get some more clarification on the code: This is pretty well implemented! The biggest cause for concern for me was this temporary tuple descriptor modification. This code switches out the new tuple descriptor for the old in order to evaluate CoerceToDomain. I’m not very familiar with this area of the code and if this has potential ramifications but I would like more clarification from the author on why this approach is safe/correct. The tests, especially after this v4 are much stronger. I was also thinking it may be worth it to add tests here for this patch on partitioned tables, to make sure the behavior holds up across all partitions. Ensuring that for example, if we alter a column type in the parent, it is consistent across all child partitions. Other than that the only other things were nit picks: The naming of the new function added “DomainHaveVolatileConstraints” was a little misleading and may lead to confusion with what the function is actually returning, because the bool value indicating volatility actually is determined from an output parameter. I like the idea of returning more information with fewer function calls though. For the second commit, in one of the earlier comments, assuming the patch is accepted should read “Previously, this was” instead of “Currently, this is”. Although I do also think this comment is more helpful for the reviewers than it is for understanding the code below it. I am still waiting to hear from Yogesh, maybe he can share his thoughts here as well. - Adi Gollamudi
