Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
> [ v2-0001-Add-primary-keys-and-unique-constraints-to-system.patch ]
I've reviewed this patch. It looks pretty solid to me, with a couple
trivial nits as mentioned below, and one bigger thing that's perhaps
in the category of bikeshedding. Namely, do we really want to prefer
using the OID indexes as the primary keys? In most cases there's some
other index that seems to me to be what a user would think of as the
pkey, for example pg_class_relname_nsp_index for pg_class or
pg_proc_proname_args_nsp_index for pg_proc. Preferring OID where it
exists is a nice simple rule, which has some attractiveness, but the
OID indexes seem to me like a lookup aid rather than the "real" object
identity.
> There is something strange going on in the misc_sanity test, as seen by
> the test output change
> -NOTICE: pg_constraint contains unpinned initdb-created object(s)
Formerly the lowest OID in pg_constraint was that of the
information_schema.cardinal_number_domain_check CHECK constraint,
which is intentionally not pinned. Now the lowest OID is that of
pg_proc_oid_index, which is pinned, hence the output change. I think
that's fine. The idea here is just to notice whether any catalogs have
been missed by setup_depend, and we have now proven that pg_constraint
wasn't missed, so it's cool.
The other catalog that is listed in the expected output, but is not
one of the ones intentionally excluded by setup_depend, is pg_rewrite.
That's because its lowest OID is a rewrite rule for a system view, which
we've intentionally made non-pinned. So that's also fine, but maybe
it'd be worth explaining it in the comment above the DO block.
Anyway, on to the minor nits:
system_constraints.sql should be removed by the maintainer-clean target
in src/backend/catalog/Makefile; perhaps also mention it in the comment
for the clean target. Also I suppose src/tools/msvc/clean.bat needs to
remove it, like it does postgres.bki.
The contents of system_constraints.sql seem pretty randomly ordered,
and I bet the order isn't stable across machines. It would be wise
to sort the entries by name to ensure we don't get inconsistencies
between different builds. (If nothing else, that could complicate
validating tarballs.)
I don't follow why the pg_seclabel, pg_shseclabel indexes aren't made
into pkeys? There's a comment claiming they "use a nondefault operator
class", but that's untrue AFAICS.
regards, tom lane
