Thanks Alvaro. On Thu, Feb 6, 2025 at 9:58 PM Alvaro Herrera <alvhe...@alvh.no-ip.org> wrote:
> Hello Rushabh, > > On 2025-Feb-06, Rushabh Lathia wrote: > > > Commit 14e87ffa5c543b5f30ead7413084c25f7735039f > > < > https://github.com/postgres/postgres/commit/14e87ffa5c543b5f30ead7413084c25f7735039f > > > > added the support for named NOT NULL constraints. We can now support > > the NOT VALID/VALID named NOT NULL constraints. > > > > This patch supports the NOT VALID and VALIDATE CONSTRAINT for name NOT > > NULL constraints. In order to achieve this patch, > > Thank you very much for working on this. > > > 1) Converted the pg_attribute.attnotnull to CHAR type, so that it can > > hold the INVALID flag for the constraint. > > This looks good to me. It'll have implications for client-side queries, > but I think they will need to adapt. One school of thought says we > should rename the column, so that every tool is forced to think about > the issue and adapt accordingly, instead of only realizing the problem > the first time they break. > I am open to suggestions here. > > > 4) Added related testcases. > > Please remember to add test cases for tables with not-valid constraint > that are not dropped at the end. That way, the pg_upgrade test will try > to process that table and we'll know if the roundtrip via pg_dump works > correctly. > Sure, will cover this. > > I haven't looked at 0002 too closely, but I think it has the right > shape. > > > 3) Support for pg_dump, where we now dumping the INVALID NOT NULL as > > separate from table schemes, just like CHECK Constraints. > > I think you copied a little bit too much of the code for check > constraints. If a constraint is accumulated in invalidnotnulloids, you > already know that it's not validated and needs to be dumped separately. > So your new query doesn't need to bring convalidated (we know it's > false). This would simplify a few lines in this new code. Also, the > pg_log_info() line is mistaken about what this block is doing. > Even though we are accumulating invalidnotnulloids, we need the condition for the invalid not null constraint, as there can me multiple not null constraint on the table - mix of valid and invalid. Initially, I tried re-using the code but it was not very clear, so thought of making at separate code for check and not null constraint. That way it's very clear. > > I think it'd be good to have NOT VALID NO INHERIT constraints in the > tests as well. Sure, will take care in the new version of the patch. > Also consider the case where the child table is created > first with a valid constraint, then the parent table is created later > with a not valid constraint -- if the pg_dump table scans find the child > first, does pg_dump do the right thing or does it try to create the > parent constraint first? yes, I tested this with the patch and pg_dump doing the right this for that scenario. > Also, what if the constraint in the child has > a different name from the constraint in the parent? This should be > pg_dump round-tripped as well. (I bet there are tons of other corner > cases here that should be verified.) Please add something to > pg_dump/t/002_pg_dump.pl. > Okay, I will add tests to pg_dump/t/002_pg_dump.pl. regards. Rushabh Lathia www.EnterpriseDB.com
