On 2021-01-17 23:07, Tom Lane wrote:
> 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.
I chose this because the notional foreign keys point to the OID.
If you design some basic business database with customer IDs, product
IDs, etc., you'd also usually make the ID the primary key, even if you
have, say, a unique constraint on the product name. But this is of
course a matter of taste to some degree.
> 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.
done
> 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.)
They follow the order in which the catalogs are processed byt genbki.pl.
This is the same order in which the catalog data and indexes are
created in postgres.bki. The Perl code to handle each of these is
conceptually the same, so that seems solid. We could order them
differently, but there is also value in keeping the catalog processing
order globally consistent.
> 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.
When I started the patch, it was text_pattern_ops, but that was a long
time ago. ;-) Fixed now.
--
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/