On Wed, Aug 31, 2022 at 4:08 PM Zhihong Yu <z...@yugabyte.com> wrote:
> > > On Wed, Aug 31, 2022 at 3:19 PM Alvaro Herrera <alvhe...@alvh.no-ip.org> > wrote: > >> So I was wrong in thinking that "this case was simple to implement" as I >> replied upthread. Doing that actually required me to rewrite large >> parts of the patch. I think it ended up being a good thing, because in >> hindsight the approach I was using was somewhat bogus anyway, and the >> current one should be better. Please find it attached. >> >> There are still a few problems, sadly. Most notably, I ran out of time >> trying to fix a pg_upgrade issue with pg_dump in binary-upgrade mode. >> I have to review that again, but I think it'll need a deeper rethink of >> how we pg_upgrade inherited constraints. So the pg_upgrade tests are >> known to fail. I'm not aware of any other tests failing, but I'm sure >> the cfbot will prove me wrong. >> >> I reluctantly added a new ALTER TABLE subcommand type, AT_SetAttNotNull, >> to allow setting pg_attribute.attnotnull without adding a CHECK >> constraint (only used internally). I would like to find a better way to >> go about this, so I may remove it again, therefore it's not fully >> implemented. >> >> There are *many* changed regress expect files and I didn't carefully vet >> all of them. Mostly it's the addition of CHECK constraints in the >> footers of many \d listings and stuff like that. At a quick glance they >> appear valid, but I need to review them more carefully still. >> >> We've had pg_constraint.conparentid for a while now, but for some >> constraints we continue to use conislocal/coninhcount. I think we >> should get rid of that and rely on conparentid completely. >> >> An easily fixed issue is that of constraint naming. >> ChooseConstraintName has an argument for passing known constraint names, >> but this patch doesn't use it and it must. >> >> One issue that I don't currently know how to fix, is the fact that we >> need to know whether a column is a row type or not (because they need a >> different null test). At table creation time that's easy to know, >> because we have the descriptor already built by the time we add the >> constraints; but if you do ALTER TABLE .. ADD COLUMN .., ADD CONSTRAINT >> then we don't. >> >> Some ancient code comments suggest that allowing a child table's NOT >> NULL constraint acquired from parent shouldn't be independently >> droppable. This patch doesn't change that, but it's easy to do if we >> decide to. However, that'd be a compatibility break, so I'd rather not >> do it in the same patch that introduces the feature. >> >> Overall, there's a lot more work required to get this to a good shape. >> That said, I think it's the right direction. >> >> -- >> Álvaro Herrera 48°01'N 7°57'E — >> https://www.EnterpriseDB.com/ >> "La primera ley de las demostraciones en vivo es: no trate de usar el >> sistema. >> Escriba un guión que no toque nada para no causar daños." (Jakob Nielsen) >> > > Hi, > For findNotNullConstraintAttnum(): > > + if (multiple == NULL) > + break; > > Shouldn't `pfree(arr)` be called before breaking ? > > +static Constraint *makeNNCheckConstraint(Oid nspid, char *constraint_name, > > You used `NN` because there is method makeCheckNotNullConstraint, right ? > I think it would be better to expand `NN` so that its meaning is easy to > understand. > > Cheers > Hi, For tryExtractNotNullFromNode , in the block for `if (rel == NULL)`: + return false; I think you meant returning NULL since false is for boolean. Cheers
