On Mon, Feb 24, 2025 at 9:54 AM Andres Freund <and...@anarazel.de> wrote:
> Hi,
>
> On 2025-02-24 05:11:48 -0500, Corey Huinker wrote:
> > Incorporating most of the feedback (I kept a few of
> > the appendNamedArgument() calls) presented over the weekend.
> >
> > * removeVersionNumStr is gone
> > * relpages/reltuples/relallvisible are now char[32] buffers in
> RelStatsInfo
> > and nowhere else (existing relpages conversion remains, however)
>
> I don't see the point. This will use more memory and if we can't get
> conversions between integers and strings right we have much bigger
> problems. The same code was used in the backend too!
>
As I see it, the point is that we're getting an input that is a string
representation from the query, and the end-goal is to convey that value
with fidelity to the destination database, so there's nothing we can do to
get us closer to the string that we already have.
I don't have benchmark numbers beyond the instinct that doing something
takes more time than doing nothing. Granted, "nothing" here means 96 bytes
of memory and 3 strncpy()s, and "something" is 24 bytes of memory, 2
atoi()s, 1 strtof() plus whatever memory and processing we do back in
converting back to strings.
> And it leads to storing relpages in two places, with different
> transformations, which doesn't seem great.
>
I didn't like that either, but balanced the ugliness of that vs the cost of
grinding the values back to where we started.
>
>
> > @@ -6921,6 +6927,7 @@ getTables(Archive *fout, int *numTables)
> > appendPQExpBufferStr(query,
> > "SELECT c.tableoid,
> c.oid, c.relname, "
> > "c.relnamespace,
> c.relkind, c.reltype, "
> > + "c.relpages, c.reltuples,
> c.relallvisible, "
> > "c.relowner, "
> > "c.relchecks, "
> > "c.relhasindex,
> c.relhasrules, c.relpages, "
>
> That query is already querying relpages a bit later in the query, so we'd
> query the column twice.
>
+1, must eliminate that duplicate.
>
>
>
> > + printfPQExpBuffer(query, "EXECUTE getAttributeStats(");
> > + appendStringLiteralAH(query, dobj->namespace->dobj.name, fout);
> > + appendPQExpBufferStr(query, ", ");
> > + appendStringLiteralAH(query, dobj->name, fout);
> > + appendPQExpBufferStr(query, "); ");
> > res = ExecuteSqlQuery(fout, query->data, PGRES_TUPLES_OK);
>
> It seems somewhat ugly that we're building an SQL string with non-trivial
> constants. It'd be better to use PQexecParams() - but I guess we don't have
> any uses of it yet in pg_dump.
>
+1, I would like to see that change.
>
> ISTM that we ought to expose the relation oid in pg_stats. This query
> would be
> simpler and faster if we could just use the oid as the predicate. Will
> take a
> while till we can rely on that, but still.
>
+1, but we will need to support this until v18 is as old as v9.2 is
now...approx 2038.
> Have you compared performance of with/without stats after these
> optimizations?
I have not.
