Re: Add starelid, attnum to pg_stats and leverage this in pg_dump

Hi,

> Per side conversation in [1], this patch exposes pg_statistic.starelid in the pg_stats view (0001)
> and then changes pg_dump to use the starelid in the queries on pg_stats rather than the
> combination of schemaname+relname, which gets a bit clumsy in bulk array fetching, and
> also leads to bad query plans against pg_stats because the security barrier is also an
> optimization barrier, which means that queries often try to hash join, which causes the
> very large table pg_statistic to be sequentially scanned, and that's a bad time. Currently
> we get around this by adding a redundant qual to the query which gooses the plan towards
> an index, and that works fine for now, but there's no guarantee that it will continue to work
> in the future, so this change brings some plan safety as well.

This alone seems like a good enough reason to include startled in
pg_stat, besides other
future use-cases this will simplify as discussed in [1]

> 0001 also exposes pg_statistic.attnum. This is no direct application of this in 0002,
> but people have often expressed surprise that pg_dump orders the
> pg_restore_attribute_stats() calls by attname rather than attnum,
> and if we wanted to change that now we could (albeit only on new versions).

This seems logical

v1-0001 comments:

1/

- nspname AS schemaname,
- relname AS tablename,
- attname AS attname,
+ n.nspname AS schemaname,
+ c.relname AS tablename,
+ s.starelid AS starelid,
+ a.attnum AS attnum,
+ a.attname AS attname,

Ideally, we would want the OID and attnum to be first columns to match
the pg_stat_activity/pg_stat_progress style, but that will be too invasive.

The new attname column should be moved after tablename, however.

2/

+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>attnum</structfield> <type>name</type>
+ (references <link
linkend="catalog-pg-attribute"><structname>pg_attribute</structname></link>.<structfield>attnum</structfield>)
+ </para>
+ <para>
+ Position of column described by this row
+ </para></entry>
+ </row>

- This <type> should be int2
- Maybe for the describtion, it should be something like:

"The number of the column, as in
<structname>pg_attribute</structname>.<structfield>attnum</structfield>"

to be close to the description in pg_attribute

3/

+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>starelid</structfield> <type>oid</type>
+ </para>
+ <para>
+ ID of the relation
+ </para></entry>
+ </row>

This should indicate it is a reference to pg_class.oid, like so:

```
(references <link
linkend="catalog-pg-class"><structname>pg_class</structname></link>.<structfield>oid</structfield>)
```

Maybe "ID of the table or index" is better, since this can only be a
table or index
for pg_stats.

I dislike the existing "pg_stats.tablename", since this can also be an
expression index.
"pg_stats.relation" with a description of "Name of table or index" is
more appropriate.
It is a change that we can possibly make in a major version. Looked
through the archives,
and did not see this being reported/discussed.

v1-0002:

I examined the version variants of getAttributeStats and all looks good,
and also ran a test on a 18 and 19 server version with the patched pg_dump
client and all looks good.

One minor comment is:

+ pg_fatal("statistics table oid information missing");

I noticed in other pg_fatal messages, we include OIDs

pg_fatal("could not find function definition for function
with OID %u",
cast->castfunc);

Should we do the same here?

[1] https://www.postgresql.org/message-id/aZ3RbTE8Racscykc@nathan

--
Sami Imseih
Amazon Web Services (AWS)