On 2023-Apr-06, Justin Pryzby wrote:
> On Thu, Apr 06, 2023 at 01:33:56AM +0200, Alvaro Herrera wrote:
> > - The forms <literal>ADD</literal> (without <literal>USING INDEX</literal>),
> > + The forms <literal>ADD</literal> (without <literal>USING INDEX</literal>, and
> > + except for the <literal>NOT NULL <replaceable>column_name</replaceable></literal>
> > + form to add a table constraint),
>
> The "except" part seems pretty incoherent to me :(
Yeah, I feared that would be the case. I can't think of a wording
that doesn't take two lines, so suggestions welcome.
I handled your other comments, except these:
> > + conrel = table_open(ConstraintRelationId, RowExclusiveLock);
>
> Should this be opened after the following error check ?
Added new code in the middle when I found a small problem, so now the
table_open is necessary there. (To wit: if we DROP NOT NULL a
constraint that is both locally defined in the table and inherited, we
should remove the "conislocal" flag and it's done. Previously, we were
throwing an error that the constraint is inherited, but that's wrong.)
> > + arr = DatumGetArrayTypeP(adatum); /* ensure not toasted */
>
> Does "arr" need to be freed ?
I see this pattern in one or two other places and we don't worry about
such small allocations too much. (I copied this code almost verbatim
from somewhere IIRC).
Anyway, I found a couple of additional minor problems when playing with
some additional corner case scenarios; I cleaned up the test cases, per
Peter. Then I realized that pg_dump support was missing completely, so
I filled that in. Sadly, the binary-upgrade mode is a bit of a mess and
thus the pg_upgrade test is failing.
--
Álvaro Herrera Breisgau, Deutschland — https://www.EnterpriseDB.com/
"La experiencia nos dice que el hombre peló millones de veces las patatas,
pero era forzoso admitir la posibilidad de que en un caso entre millones,
las patatas pelarían al hombre" (Ijon Tichy)