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
