Re: Replacing pg_depend PIN entries with a fixed range check

Hi,

On 2021-04-14 21:43:28 -0400, Tom Lane wrote:
> In [1] Andres and I speculated about whether we really need all
> those PIN entries in pg_depend. Here is a draft patch that gets
> rid of them.

Yay.

> There are a couple of objects, namely template1 and the public
> schema, that are in the catalog .dat files but are not supposed
> to be pinned. The existing code accomplishes that by excluding them
> (in two different ways :-() while filling pg_depend. This patch
> just hard-wires exceptions for them in IsPinnedObject(), which seems
> to me not much uglier than what we had before. The existing code
> also handles pinning of the standard tablespaces in an idiosyncratic
> way; I just dropped that and made them be treated as pinned.

Hm, maybe we ought to swap template0 and template1 instead? I.e. have
template0 be in pg_database.dat and thus get a pinned oid, and then
create template1, postgres etc from that?

I guess we could also just create public in initdb.

Not that it matters much, having those exceptions doesn't seem too bad.

> Anyway, as to concrete results:
>
> * pg_depend's total relation size, in a freshly made database,
> drops from 1269760 bytes to 368640 bytes.

Nice!

> I didn't try to reproduce the original performance bottleneck
> that was complained of in [1], but that might be fun to check.

I hope it's not reproducible as is, because I hopefully did fix the bug
leading to it ;)

> +bool
> +IsPinnedObject(Oid classId, Oid objectId)
> +{
> + /*
> + * Objects with OIDs above FirstUnpinnedObjectId are never pinned. Since
> + * the OID generator skips this range when wrapping around, this check
> + * guarantees that user-defined objects are never considered pinned.
> + */
> + if (objectId >= FirstUnpinnedObjectId)
> + return false;
> +
> + /*
> + * Large objects are never pinned. We need this special case because
> + * their OIDs can be user-assigned.
> + */
> + if (classId == LargeObjectRelationId)
> + return false;
> +

Huh, shouldn't we reject that when creating them? IIRC we already use
oid range checks in a bunch of places? I guess you didn't because of
dump/restore concerns?

Greetings,

Andres Freund