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 1rZl2F-006Gcq-6r for pgsql-hackers@arkaria.postgresql.org; Tue, 13 Feb 2024 05:08:48 +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 1rZl1D-00Fa1L-MX for pgsql-hackers@arkaria.postgresql.org; Tue, 13 Feb 2024 05:07:43 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.94.2) (envelope-from ) id 1rZl1D-00Fa1A-4E for pgsql-hackers@lists.postgresql.org; Tue, 13 Feb 2024 05:07:43 +0000 Received: from mail-io1-xd2c.google.com ([2607:f8b0:4864:20::d2c]) by makus.postgresql.org with esmtps (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_128_GCM_SHA256 (Exim 4.94.2) (envelope-from ) id 1rZl19-006U0F-TL for pgsql-hackers@lists.postgresql.org; Tue, 13 Feb 2024 05:07:41 +0000 Received: by mail-io1-xd2c.google.com with SMTP id ca18e2360f4ac-7bb5fda069bso192977639f.0 for ; Mon, 12 Feb 2024 21:07:39 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1707800859; x=1708405659; 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=Ndy/VyHqNiCsgTLw1D1CF+CDD7hzjgSKNnQKOGmG8t8=; b=OCBOjXe9LZ3qGjIlVsfT8r9cLSr8w/944rItwbqBCeGHivf0nF0CqoA8t5TaYXQ2YY zmqMORaDmomnLLbQIFJSNwMjRtGpgo+Ev6nMCYeoUEcAHVfbpVWSyTu0Wad4lnjxfosw iMTtcmga31Tqpj9hBCfIAfuO1BWfVyIA/tgvOynyvGHzu7vE2hh8KtpgQp0aunqTI7L7 t4tyG7/aP7o/tNxET4kePVjW1bRgU8WXwQNbfbMAm2LRh+7YUe1ZSyP0QypRRng9jsK9 u0WaRAIQIPExRmRoUtkDbIRBuQVsDPPA+j1wTDhDfVD3rxGIqbTQEbvGmhVL23JK6kzL 2OGg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707800859; x=1708405659; 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=Ndy/VyHqNiCsgTLw1D1CF+CDD7hzjgSKNnQKOGmG8t8=; b=b3nBpcGh4HpfHM8kWOb+UHmR0HJSOhN0hGsAesU/Xu7T077JBD4Eum4fjGRHVkLB3G wiEwrHXNdsleyJ3TC7JO7/5/Lf7BnASgyJg9zXFX+cwwLEaMMNSLwny8ku2kbZlIx7Qe tilPlVXoOLjHaGL8Joh0P+dSIPJf5O26LRRSoDZBoiW15f/+3X7mf+PV6XKG8+mtmCDF g69w6IHf3HUf6UvBaYcuzHTfJGCoi4Mzl3WNi8A8oaZxCDMhCdUf8FKaGovoLPoh5clp RB0sSP12vrg4wKCAzxNwehiRPd2qP2iDDNYUruPnJFtWmcTuWOn4revI14fveof1GmY7 WEnw== X-Forwarded-Encrypted: i=1; AJvYcCUVz//vfvJUUkGBPtsjLA/fAaQYwqcpoZDoHayIMfTcEnDhVwFZIgqbj1hzI3SLcEAulkLQtv3IOWFuzK6hvlocOygRNrcyHNhhg0gyueO3OzlY X-Gm-Message-State: AOJu0Yygm4Ui5Ngen2h6bLnLOdsmDbslkh5i74Xrb+V9Kq9Gp6pgAi1T mgyrujiop/YOegkpjPOxznE1mdmsVq7AIjjsefl9qLhSpZWTYnSkkfDcZGhplmOjebsMNGnSGiQ 69HNBBpWkj5Ku2cyQfBS2e20M3Z8= X-Google-Smtp-Source: AGHT+IECKmyvzuEyDl//YYDdTxABDod2tR3Uuk13sF/A7NSIYUoI21idy2e4x7kdZCk1IEAn5m1ljKKR5bZAHzpGoJw= X-Received: by 2002:a92:da0d:0:b0:363:c919:2716 with SMTP id z13-20020a92da0d000000b00363c9192716mr10213311ilm.20.1707800859032; Mon, 12 Feb 2024 21:07:39 -0800 (PST) MIME-Version: 1.0 References: <76596388-6fe6-0baf-351d-734458a46d76@enterprisedb.com> <6b942362-cd19-4e1c-b6da-f230c3f3d9a1@enterprisedb.com> In-Reply-To: <6b942362-cd19-4e1c-b6da-f230c3f3d9a1@enterprisedb.com> From: Corey Huinker Date: Tue, 13 Feb 2024 00:07:26 -0500 Message-ID: Subject: Re: Statistics Import and Export To: Tomas Vondra Cc: Peter Smith , Ashutosh Bapat , pgsql-hackers@lists.postgresql.org Content-Type: multipart/alternative; boundary="0000000000008cc41006113c5f15" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --0000000000008cc41006113c5f15 Content-Type: text/plain; charset="UTF-8" > > Also, it says "statistics are replaced" but it's quite clear if that > applies only to matching statistics or if all stats are deleted first > and then the new stuff is inserted. (FWIW remove_pg_statistics clearly > deletes all pre-existing stats). > All are now deleted first, both in the pg_statistic and pg_statistic_ext_data tables. The previous version was taking a more "replace it if we find a new value" approach, but that's overly complicated, so following the example set by extended statistics seemed best. > - import_pg_statistics: I somewhat dislike that we're passing arguments > as datum[] array - it's hard to say what the elements are expected to > be, etc. Maybe we should expand this, to make it clear. How do we even > know the array is large enough? > Completely fair. Initially that was done with the expectation that the array would be the same for both regular stats and extended stats, but that was no longer the case. > - I don't quite understand why we need examine_rel_attribute. It sets a > lot of fields in the VacAttrStats struct, but then we only use attrtypid > and attrtypmod from it - so why bother and not simply load just these > two fields? Or maybe I miss something. > I think you're right, we don't need it anymore for regular statistics. We still need it in extended stats because statext_store() takes a subset of the vacattrstats rows as an input. Which leads to a side issue. We currently have 3 functions: examine_rel_attribute and the two varieties of examine_attribute (one in analyze.c and the other in extended stats). These are highly similar but just different enough that I didn't feel comfortable refactoring them into a one-size-fits-all function, and I was particularly reluctant to modify existing code for the ANALYZE path. > > - examine_rel_attribute can return NULL, but get_attrinfo does not check > for NULL and just dereferences the pointer. Surely that can lead to > segfaults? > Good catch, and it highlights how little we need VacAttrStats for regular statistics. > > - validate_no_duplicates and the other validate functions would deserve > a better docs, explaining what exactly is checked (it took me a while to > realize we check just for duplicates), what the parameters do etc. > Those functions are in a fairly formative phase - I expect a conversation about what sort of validations we want to do to ensure that the statistics being imported make sense, and under what circumstances we would forego some of those checks. > > - Do we want to make the validate_ functions part of the public API? I > realize we want to use them from multiple places (regular and extended > stats), but maybe it'd be better to have an "internal" header file, just > like we have extended_stats_internal? > I see no need to have them be a part of the public API. Will move. > > - I'm not sure we do "\set debug f" elsewhere. It took me a while to > realize why the query outputs are empty ... > That was an experiment that rose out of the difficulty in determining _where_ a difference was when the set-difference checks failed. So far I like it, and I'm hoping it catches on. > > > 0002 > > - I'd rename create_stat_ext_entry to statext_create_entry. > > - Do we even want to include OIDs from the source server? Why not to > just have object names and resolve those? Seems safer - if the target > server has the OID allocated to a different object, that could lead to > confusing / hard to detect issues. > The import functions would obviously never use the imported oids to look up objects on the destination system. Rather, they're there to verify that the local object oid matches the exported object oid, which is true in the case of a binary upgrade. The export format is an attempt to export the pg_statistic[_ext_data] for that object as-is, and, as Tom suggested, let the import function do the transformations. We can of course remove them if they truly have no purpose for validation. > > - What happens if we import statistics which includes data for extended > statistics object which does not exist on the target machine? > The import function takes an oid of the object (relation or extstat object), and the json payload is supposed to be the stats for ONE corresponding object. Multiple objects of data really don't fit into the json format, and statistics exported for an object that does not exist on the destination system would have no meaningful invocation. I envision the dump file looking like this CREATE TABLE public.foo (....); SELECT pg_import_rel_stats('public.foo'::regclass, , option flag, option flag); So a call against a nonexistent object would fail on the regclass cast. > > - pg_import_ext_stats seems to not use require_match_oids - bug? > I haven't yet seen a good way to make use of matching oids in extended stats. Checking matching operator/collation oids would make sense, but little else. > > > 0003 > > - no SGML docs for the new tools? > Correct. I foresee the export tool being folded into pg_dump(), and the import tool going away entirely as psql could handle it. > > - The help() seems to be wrong / copied from "clusterdb" or something > like that, right? > Correct, for the reason above. > > > > pg_import_rel_stats matches up local columns with the exported stats by > > column name, not attnum. This allows for stats to be imported when > columns > > have been dropped, added, or reordered. > > > > Makes sense. What will happen if we try to import data for extended > statistics (or index) that does not exist on the target server? > One of the parameters to the function is the oid of the object that is the target of the stats. The importer will not seek out objects with matching names and each JSON payload is limited to holding one object, though clearly someone could encapsulate the existing format in a format that has a manifest of objects to import. > > > pg_import_ext_stats can also handle column reordering, though it > currently > > would get confused by changes in expressions that maintain the same > result > > data type. I'm not yet brave enough to handle importing nodetrees, nor > do I > > think it's wise to try. I think we'd be better off validating that the > > destination extended stats object is identical in structure, and to fail > > the import of that one object if it isn't perfect. > > > > Yeah, column reordering is something we probably need to handle. The > stats order them by attnum, so if we want to allow import on a system > where the attributes were dropped/created in a different way, this is > necessary. I haven't tested this - is there a regression test for this? > The overlong transformation SQL starts with the object to be imported (the local oid was specified) and it 1. grabs all the attributes (or exprs, for extended stats) of that object. 2. looks for columns/exprs in the exported json for an attribute with a matching name 3. takes the exported attnum of that exported attribute for use in things like stdexprs 4. looks up the type, collation, and operators for the exported attribute. So we get a situation where there might not be importable stats for an attribute of the destination table, and we'd import nothing for that column. Stats for exported columns with no matching local column would never be referenced. Yes, there should be a test of this. > I agree expressions are hard. I don't think it's feasible to import > nodetree from other server versions, but why don't we simply deparse the > expression on the source, and either parse it on the target (and then > compare the two nodetrees), or deparse the target too and compare the > two deparsed expressions? I suspect the deparsing may produce slightly > different results on the two versions (causing false mismatches), but > perhaps the deparse on source + parse on target + compare nodetrees > would work? Haven't tried, though. > > > Export formats go back to v10. > > > > Do we even want/need to go beyond 12? All earlier versions are EOL. > True, but we had pg_dump and pg_restore stuff back to 7.x until fairly recently, and a major friction point in getting customers to upgrade their instances off of unsupported versions is the downtime caused by an upgrade, why wouldn't we make it easier for them? --0000000000008cc41006113c5f15 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Also, it says "statistics are replaced" but it= 's quite clear if that
applies only to matching statistics or if all stats are deleted first
and then the new stuff is inserted. (FWIW remove_pg_statistics clearly
deletes all pre-existing stats).

All are now delet= ed first, both in the pg_statistic and pg_statistic_ext_data tables. The pr= evious version was taking a more "replace it if we find a new value&qu= ot; approach, but that's overly complicated, so following the example s= et by extended statistics seemed best.

=C2=A0
- import_pg_statistics: I somewhat dislike= that we're passing arguments
as datum[] array - it's hard to say what the elements are expected to be, etc. Maybe we should expand this, to make it clear. How do we even
know the array is large enough?

Completely fair. I= nitially that was done with the expectation that the array would be the sam= e for both regular stats and extended stats, but that was no longer the cas= e.
=C2=A0
- I don= 't quite understand why we need examine_rel_attribute. It sets a
lot of fields in the VacAttrStats struct, but then we only use attrtypid and attrtypmod from it - so why bother and not simply load just these
two fields? Or maybe I miss something.
=C2=A0
I thi= nk you're right, we don't need it anymore for=C2=A0regular=C2=A0sta= tistics. We still need it in extended stats because=C2=A0statext_store() ta= kes a subset of the vacattrstats rows as an input.

Which leads to a = side issue. We currently have 3 functions: examine_rel_attribute and the tw= o varieties of examine_attribute (one in analyze.c and the other in extende= d stats). These are highly similar but=C2=A0just different enough that I di= dn't feel comfortable refactoring them into a one-size-fits-all functio= n, and I was particularly reluctant to modify existing code for the ANALYZE= path.
=C2=A0

- examine_rel_attribute can return NULL, but get_attrinfo does not check for NULL and just dereferences the pointer. Surely that can lead to
segfaults?

Good catch, and it highlights how littl= e we need VacAttrStats for regular statistics.
=C2=A0

- validate_no_duplicates and the other validate functions would deserve
a better docs, explaining what exactly is checked (it took me a while to realize we check just for duplicates), what the parameters do etc.

Those functions are in a fairly formative phase - I expect= a conversation about what sort of validations we want to do to ensure that= the statistics being imported make sense, and under what circumstances we = would forego some of those checks.
=C2=A0

- Do we want to make the validate_ functions part of the public API? I
realize we want to use them from multiple places (regular and extended
stats), but maybe it'd be better to have an "internal" header= file, just
like we have extended_stats_internal?

I see no nee= d to have them be a part of the public API. Will move.
=C2=A0

- I'm not sure we do "\set debug f" elsewhere. It took me a w= hile to
realize why the query outputs are empty ...

That w= as an experiment that rose out of the difficulty in determining _where_ a d= ifference was when the set-difference checks failed. So far I like it, and = I'm hoping it catches on.

=C2=A0


0002

- I'd rename create_stat_ext_entry to statext_create_entry.

- Do we even want to include OIDs from the source server? Why not to
just have object names and resolve those? Seems safer - if the target
server has the OID allocated to a different object, that could lead to
confusing / hard to detect issues.

The = import functions would obviously never use the imported oids to look up obj= ects on the destination system. Rather, they're there to verify that th= e local object oid matches the exported object oid, which is true in the ca= se of a binary upgrade.

The export format is an attempt to export th= e pg_statistic[_ext_data] for that object as-is, and, as Tom suggested, let= the import function do the transformations. We can of course remove them i= f they truly have no purpose for validation.
=C2=A0

- What happens if we import statistics which includes data for extended
statistics object which does not exist on the target machine?

The import function takes an oid of the object (relation or ext= stat object), and the json payload is supposed to be the stats for ONE corr= esponding object. Multiple objects of data really don't fit into the js= on format, and statistics exported for an object that does not exist on the= destination system would have no meaningful invocation. I envision the dum= p file looking like this

=C2=A0 =C2=A0 CREATE TABLE public.foo (....= );

=C2=A0 =C2=A0 SELECT pg_import_rel_stats('public.foo'::re= gclass, <json blob>, option flag, option flag);

So a call agai= nst a nonexistent object would fail on the regclass cast.
=C2=A0

- pg_import_ext_stats seems to not use require_match_oids - bug?

I haven't yet seen a good way to make use of matching oi= ds in extended stats. Checking matching operator/collation oids would make = sense, but little else.
=C2=A0


0003

- no SGML docs for the new tools?

Correct. I fores= ee the export tool being folded into pg_dump(), and the import tool going a= way entirely as psql could handle it.
=C2=A0

- The help() seems to be wrong / copied from "clusterdb" or somet= hing
like that, right?

Correct, for the reason above.
=C2=A0
> > pg_import_rel_stats matches up local columns with the exported stats b= y
> column name, not attnum. This allows for stats to be imported when col= umns
> have been dropped, added, or reordered.
>

Makes sense. What will happen if we try to import data for extended
statistics (or index) that does not exist on the target server?

One of the parameters to the function is the oid of the objec= t that is the target of the stats. The importer will not seek out objects w= ith matching names and each JSON payload is limited to holding one object, = though clearly someone could encapsulate the existing format in a format th= at has a manifest of objects to import.
=C2=A0

> pg_import_ext_stats can also handle column reordering, though it curre= ntly
> would get confused by changes in expressions that maintain the same re= sult
> data type. I'm not yet brave enough to handle importing nodetrees,= nor do I
> think it's wise to try. I think we'd be better off validating = that the
> destination extended stats object is identical in structure, and to fa= il
> the import of that one object if it isn't perfect.
>

Yeah, column reordering is something we probably need to handle. The
stats order them by attnum, so if we want to allow import on a system
where the attributes were dropped/created in a different way, this is
necessary. I haven't tested this - is there a regression test for this?=

The overlong transformation=C2=A0SQL s= tarts with the object to be imported (the local oid was specified) and it

1. grabs all the attributes (or exprs, for extended= stats) of that object.
2. looks for columns/exprs in the exported json = for an attribute with a matching name
3. takes the exported attnu= m of that exported attribute for use in things like stdexprs
4. looks up= the type, collation, and operators for the exported attribute.

So w= e get a situation where there might not be importable stats for an attribut= e of the destination table, and we'd import nothing for that column. St= ats for exported columns with no matching local column would never be refer= enced.

Yes, there should be a test of this.

I agree expressions are hard. I don't think it's feasible to import=
nodetree from other server versions, but why don't we simply deparse th= e
expression on the source, and either parse it on the target (and then
compare the two nodetrees), or deparse the target too and compare the
two deparsed expressions? I suspect the deparsing may produce slightly
different results on the two versions (causing false mismatches), but
perhaps the deparse on source + parse on target + compare nodetrees
would work? Haven't tried, though.

> Export formats go back to v10.
>

Do we even want/need to go beyond 12? All earlier versions are EOL.

True, but we had pg_dump and pg_restore stuff back to 7.x= until fairly recently, and a major friction point in getting customers to = upgrade their instances off of unsupported versions is the downtime caused = by an upgrade, why wouldn't we make it easier for them?

--0000000000008cc41006113c5f15--