Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tmdUP-008JLE-7q for pgsql-hackers@arkaria.postgresql.org; Mon, 24 Feb 2025 18:47:37 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.94.2) (envelope-from ) id 1tmdUN-006P53-EO for pgsql-hackers@arkaria.postgresql.org; Mon, 24 Feb 2025 18:47:35 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1tmdUN-006P4v-2i for pgsql-hackers@lists.postgresql.org; Mon, 24 Feb 2025 18:47:35 +0000 Received: from mail-io1-xd31.google.com ([2607:f8b0:4864:20::d31]) by magus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.96) (envelope-from ) id 1tmdUK-000aFW-27 for pgsql-hackers@lists.postgresql.org; Mon, 24 Feb 2025 18:47:34 +0000 Received: by mail-io1-xd31.google.com with SMTP id ca18e2360f4ac-85592116ca4so375985039f.1 for ; Mon, 24 Feb 2025 10:47:32 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1740422851; x=1741027651; darn=lists.postgresql.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=03fHmtp3lRcft8kTB6n9a8RpuQEy5UaNfB0/e2vbfc8=; b=QciclmhJVB4g9BLlDIEsKoOUGTLdflomYbcBplxBFGLNs7MO8EVyPhVaG0AaTizsCq a1Rka1qwS6eqogmQra2WlWqYXVvkOv1Rewm7YUKWRPZgaqqCIQ1JqJ5ktVZnrHJnMxTv JcNqey2/Bi/ITTH3iuWM+7L7VoT4rVvVyP3w76b6oNPqM+GI0Nq0KsPkxbb4kxnJS31y SLn6KiM1eFLtUOeKIKDM9y472mKxN/7Qc1aaqq/oWITo7rF0NgZJOrZz97LJkdW9bts2 WbKsGa/k90DU+U5XsArXluPEpX3OLscSdLSK+5kGNUefOP+jpMHSV/XRGfI9gLuKl+4G /TZw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1740422851; x=1741027651; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=03fHmtp3lRcft8kTB6n9a8RpuQEy5UaNfB0/e2vbfc8=; b=HBOjtd4AiX/JtiLFZlzAFIo3r3sOPBUrvzm3K4aixBhiTutLiNEDxooWrZa0vvR9cZ Zt+o+hhjoqYbZikRGvRcoN3jHXtBSgjHA5+F+IGt6JCIuFQSOEgPA5QcQ8CoIcDi3Pfh OxOiA7EUT95Yp5mcqSoKDSILADorMS/Cs7lc8FbnAI3jJRIxQUaAHaM0XF2SFpskB7Mt blWGwgFjnPhbv4KygLQr1H4I3rU1e0D+bFeOIyEhGt4Sy6YYa98/pF6dRwFlO35K0iRO cg4NLXBSMIs1nfUxJPKem4FRk7AfNMhhlxmBiDqzwE0DNqcRlEWdQX+0Qd8Q+jVU8YYi g8gQ== X-Forwarded-Encrypted: i=1; AJvYcCXCQVFLeWqPWzx71HbOMaZW7q02hoY+/oNeRFPg6i/Y/emD6auDHtK5oE8axHmMhVUUKOqwMZXd3vtXFFkQ@lists.postgresql.org X-Gm-Message-State: AOJu0Yy1mKhfK9R344b520dRKIfKNRUiqsWZDEpTOa7psU4RwQxEZEQH 0kyR//2T/NE1XpzbueVCfg+Pa71EC0IAFZpODCiZh2BXV6Jti/wZK5h3B/AlMr7gepJ7HOTJBLK pmgWeaTv9sV3OabudFo2HOsVJd3k= X-Gm-Gg: ASbGncuQm+YJElisewAVuWvDNgimXNHzRGpOxmlIOpphk7JUGZmeb0/GdjU67QrXi6E B8ibOaZdJ4MpFSI/phJhXfJhmpoZMYmeSIJJn0oq1hgFeNXuqnLDXBFfp1NFvlwaIvBl/C/c15y VI9xqmKJ4= X-Google-Smtp-Source: AGHT+IEKnv1y0WNwqs3AgiNOGsZVJ3+8ORGVFrw1XajeI5smklGB98oFuHDo1xB+zIOBBeQc8Ei95KG/a4wb0woFtsE= X-Received: by 2002:a05:6e02:194b:b0:3d0:10a6:99aa with SMTP id e9e14a558f8ab-3d2cae4e70dmr128398315ab.4.1740422851091; Mon, 24 Feb 2025 10:47:31 -0800 (PST) MIME-Version: 1.0 References: <3670503.1740173078@sss.pgh.pa.us> <3728741.1740178078@sss.pgh.pa.us> <97b451228227c555be1a4f79c4a62ddec9a74f06.camel@j-davis.com> <1457469.1740419458@sss.pgh.pa.us> In-Reply-To: <1457469.1740419458@sss.pgh.pa.us> From: Corey Huinker Date: Mon, 24 Feb 2025 13:47:19 -0500 X-Gm-Features: AWEUYZmUk3j_aML2mL4dUyXyOExN4Yv8NV6oi3ymChNKERvQx0QUnk1Z8hYusXU Message-ID: Subject: Re: Statistics Import and Export To: Tom Lane Cc: Andres Freund , Jeff Davis , Michael Paquier , jian he , Nathan Bossart , Bruce Momjian , Matthias van de Meent , Magnus Hagander , Stephen Frost , Ashutosh Bapat , Peter Smith , PostgreSQL Hackers , alvherre@alvh.no-ip.org Content-Type: multipart/alternative; boundary="000000000000cc6a15062ee7c56f" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --000000000000cc6a15062ee7c56f Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Mon, Feb 24, 2025 at 12:51=E2=80=AFPM Tom Lane wrote= : > Andres Freund writes: > > On 2025-02-24 05:11:48 -0500, Corey Huinker wrote: > >> * 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! > > I don't like that either. But there's a bigger problem with 0002: > it's still got mostly table-driven output. I've been working on > fixing the problem discussed over in the -committers thread about how > we need to identify index-expression columns by number not name [1]. > There doesn't seem to be any way around it, but it will slightly complicate the dump-ing side of things, in that we need to either: a) switch to attnums for index expressions and keep attname calls for everything else. b) track what the attnum will be on the destination side, which will be different when we're not doing a binary upgrade and there are any preceding dropped columns. The patch Tom provided opens the door for option "a", and I'm inclined to take it. > It's not too awful in the backend (WIP patch attached), but > getting appendAttStatsImport to do it seems like a complete disaster, > and this patch fails to make that any easier. It'd be much better > if you gave up on that table-driven business and just open-coded the > handling of the successive output values as was discussed upthread. > Can do. > I don't think the table-driven approach has anything to recommend it > anyway. It requires keeping att_stats_arginfo[] in sync with the > query in getAttStatsExportQuery, an extremely nonobvious (and > undocumented) connection. Personally I would nuke the separate > getAttStatsExportQuery and appendAttStatsImport functions altogether, > and have one function that executes a query and immediately interprets > the PGresult. > +1, though that comes at the cost of shutting off the possibility of a mass fetch from pg_stats without also rendering the pg_restore_attribute_stats calls at the same time. > > Also, while working on the attached, I couldn't help forming the > opinion that we'd be better off to nuke pg_set_attribute_stats() > from orbit and require people to use pg_restore_attribute_stats(). > pg_set_attribute_stats() would be fine if we had a way to force > people to call it with only named-argument notation, but we don't. > So I'm afraid that its existence will encourage people to rely > on a specific parameter order, and then they'll whine if we > add/remove/reorder parameters, as indeed I had to do below. > They've always had split goals. To recap for people just joining the show, the "set" family had the following properties: 1. transactional, even for pg_class 2. assumes all stats given are relevant and correct for current db version 3. guaranteed to ERROR if any parameter doesn't check out 4. unstable call signature, can and will change to match the realities of the current version 5. intended for planner experiments and fuzzing and the "restore" family has the following properties: 1. will inplace update pg_class to avoid table bloat 2. states the version from whence the stats came, so that adjustments can be made to suit the current db version, up to and including rejecting that particular statistic 3. attempts to sidestep errors with WARNINGs so as not to kill a restore 4. stable but highly fluid kwargs-ish call signature 5. intended to be machine generated and used only in restore/upgrade The attnum change certainly throws a wrench into that, and if we get rid of the setter functions then we will need to (re)introduce parameters to indicate our choice for properties 1 and 3. I suppose we could use the existence or non-existence of the "version" parameter as an indicator of which mode we want (if it exists, we want WARNINGS and inplace updates, if not we want pure transactional and ERROR at the first problem), but I'm not certain that proxy will hold true in the future. > BTW, I pushed the 0003 patch with minor adjustments. > Thanks! --000000000000cc6a15062ee7c56f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
On Mon, Feb 24, 2025 at 12:51=E2=80=AFPM = Tom Lane <tgl@sss.pgh.pa.us>= wrote:
Andres Freund <andres@anarazel.de> writes:
> On 2025-02-24 05:11:48 -0500, Corey Huinker wrote:
>> * relpages/reltuples/relallvisible are now char[32] buffers in Rel= StatsInfo
>> 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!

I don't like that either.=C2=A0 But there's a bigger problem with 0= 002:
it's still got mostly table-driven output.=C2=A0 I've been working = on
fixing the problem discussed over in the -committers thread about how
we need to identify index-expression columns by number not name [1].

There doesn't seem to be any way around i= t, but it will slightly=C2=A0complicate the dump-ing side of things, in tha= t we need to either:

a) switch to attnums=C2=A0for index expressions= and keep attname calls for everything else.

b) track what th= e attnum will be on the destination side,=C2=A0which will be different when= we're not doing a binary upgrade and there are=C2=A0any preceding drop= ped columns.

The patch Tom provided opens the door for option "= a", and I'm inclined to take it.

=C2=A0
It's not too awful in the backend (WIP patch attached), but
getting appendAttStatsImport to do it seems like a complete disaster,
and this patch fails to make that any easier.=C2=A0 It'd be much better=
if you gave up on that table-driven business and just open-coded the
handling of the successive output values as was discussed upthread.

Can do.


I don't think the table-driven approach has anything to recommend it anyway.=C2=A0 It requires keeping att_stats_arginfo[] in sync with the
query in getAttStatsExportQuery, an extremely nonobvious (and
undocumented) connection.=C2=A0 Personally I would nuke the separate
getAttStatsExportQuery and appendAttStatsImport functions altogether,
and have one function that executes a query and immediately interprets
the PGresult.

+1, though that comes at the cost of= shutting off the possibility of a mass fetch from pg_stats without also re= ndering the pg_restore_attribute_stats calls at the same time.
= =C2=A0

Also, while working on the attached, I couldn't help forming the
opinion that we'd be better off to nuke pg_set_attribute_stats()
from orbit and require people to use pg_restore_attribute_stats().
pg_set_attribute_stats() would be fine if we had a way to force
people to call it with only named-argument notation, but we don't.
So I'm afraid that its existence will encourage people to rely
on a specific parameter order, and then they'll whine if we
add/remove/reorder parameters, as indeed I had to do below.

They've always had split goals. To recap for peopl= e just joining the show, the "set" family had the following prope= rties:

1. transactional, even for pg_class
2. assumes all stats g= iven are relevant and correct for current db version
3. guaranteed to ER= ROR if any parameter doesn't check out
4. unstable call signature, c= an and will change to match the realities of the current version
= 5. intended for planner experiments and fuzzing

and the "restor= e" family has the following properties:

1. will inplace update = pg_class to avoid table bloat
2. states the version from whence the stat= s came, so that adjustments can be made to suit the current db version, up = to and including rejecting that particular statistic
3. attempts = to sidestep errors with WARNINGs so as not to kill a restore
4. stable b= ut highly fluid kwargs-ish call signature
5. intended to be machi= ne generated and used only in restore/upgrade

The = attnum change certainly throws a wrench into that, and if we get rid of the= setter functions then we will need to (re)introduce parameters to indicate= our choice for properties 1 and 3. I suppose we could use the existence or= non-existence of the "version" parameter as an indicator of whic= h mode we want (if it exists, we want WARNINGS and inplace updates, if not = we want pure transactional and ERROR at the first problem), but I'm not= certain that proxy will hold true in the future.
=C2=A0
BTW, I pushed the 0003 patch with m= inor adjustments.

Thanks!=C2=A0
--000000000000cc6a15062ee7c56f--