Обсуждение: pgsql: Update pg_upgrade test for reg* to include regrole andregnamesp
Update pg_upgrade test for reg* to include regrole and regnamespace. When the regrole (0c90f6769) and regnamespace (cb9fa802b) types were added in 9.5, pg_upgrade's check for reg* types wasn't updated. While regrole currently is safe, regnamespace is not. It seems unlikely that anybody uses regnamespace inside catalog tables across a pg_upgrade, but the tests should be correct nevertheless. While at it, reorder the types checked in the query to be alphabetical. Otherwise it's annoying to compare existing and tested for types. Author: Andres Freund Discussion: https://postgr.es/m/037e152a-cb25-3bcb-4f35-bdc9988f8204@2ndQuadrant.com Backpatch: 9.5-, as regrole/regnamespace Branch ------ master Details ------- https://git.postgresql.org/pg/commitdiff/ac218aa4f6dff6dfb4bf2675b2d9f2f23da6a7c5 Modified Files -------------- src/bin/pg_upgrade/check.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-)
Andres Freund <andres@anarazel.de> writes:
> Update pg_upgrade test for reg* to include regrole and regnamespace.
This has broken all the back-branch cross-version upgrade tests,
to go along with the HEAD cases you already broke :-(
regards, tom lane
Hi, On 2018-11-27 00:43:43 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> writes: > > Update pg_upgrade test for reg* to include regrole and regnamespace. > > This has broken all the back-branch cross-version upgrade tests, Hm, darn it. Gotta rewrite the query to reference those new types without the regtype casts (as to_regtype without errors doesn't exist far enough back). Seems easier than having a version check for the different types. > to go along with the HEAD cases you already broke :-( The cross-version bug around pg_largeobject_metadata ought to be fixed now. I'm hoping for a response by Andrew about the ordering bit, you're welcome to opine on that too. I see that prairiedog https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog&dt=2018-11-27%2005%3A30%3A39 failed just it's last run, but I'm not clear whether that's just a random failure, or something I can't don't recognize without further information. Greetings, Andres Freund
On 11/27/18 1:32 AM, Andres Freund wrote: > > The cross-version bug around pg_largeobject_metadata ought to be fixed > now. I'm hoping for a response by Andrew about the ordering bit, you're > welcome to opine on that too. > > I see that prairiedog > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=prairiedog&dt=2018-11-27%2005%3A30%3A39 > failed just it's last run, but I'm not clear whether that's just a > random failure, or something I can't don't recognize without further > information. The report is that the dumps differ. My bet is that this is another instance of the LO ordering issue. I can handle that in the cross-version tests because they are completely under my control (see <https://github.com/PGBuildFarm/client-code/commit/9ca42ac1783a8cf99c73b4f7c52bd05a6024669d>) , but not in the builtin test, which has this: if diff "$temp_root"/dump1.sql "$temp_root"/dump2.sql >/dev/null; then echo PASSED exit 0 else echo "Files $temp_root/dump1.sql and $temp_root/dump2.sql differ" echo "dumps were not identical" exit 1 fi So that's all we know. Maybe we should change the script to output the diff, or at least some of it, say the first 1000 lines. In any case, I think we will probably need to bite the bullet and have pg_dump render LOs in OID order. Not sure if that needs to be always on, or an option. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes:
> In any case, I think we will probably need to bite the bullet and have
> pg_dump render LOs in OID order.
+1. Looking at the relevant code:
/*
* Currently, we re-fetch all BLOB OIDs using a cursor. Consider scanning
* the already-in-memory dumpable objects instead...
*/
if (fout->remoteVersion >= 90000)
blobQry = "DECLARE bloboid CURSOR FOR SELECT oid FROM pg_largeobject_metadata";
else
blobQry = "DECLARE bloboid CURSOR FOR SELECT DISTINCT loid FROM pg_largeobject";
I suspect that the original expectation was that SELECT DISTINCT would
deliver the blob OIDs in sorted order, and nobody noticed when hash-based
DISTINCT broke that, and then the pg_largeobject_metadata patch figured
that it didn't need to preserve a guarantee that wasn't there.
We have taken pains in the past to ensure consistent dump ordering, and
it seems to me the lack of that here is just an oversight. It's kinda
surprising that it hasn't bit us before.
> Not sure if that needs to be always on,
> or an option.
We have no existing option that says "you can skip ordering my dump
consistently". I see no need for one here.
The easy fix is to add "ORDER BY 1" to both of the above queries.
If anyone is concerned about the performance of that, they could
go try to do what the comment suggests, but I'm not personally
excited about that.
regards, tom lane
Hi, On 2018-11-27 10:47:29 -0500, Tom Lane wrote: > Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: > > In any case, I think we will probably need to bite the bullet and have > > pg_dump render LOs in OID order. > > +1. Looking at the relevant code: > > /* > * Currently, we re-fetch all BLOB OIDs using a cursor. Consider scanning > * the already-in-memory dumpable objects instead... > */ > if (fout->remoteVersion >= 90000) > blobQry = "DECLARE bloboid CURSOR FOR SELECT oid FROM pg_largeobject_metadata"; > else > blobQry = "DECLARE bloboid CURSOR FOR SELECT DISTINCT loid FROM pg_largeobject"; Done. Thanks for chiming in. Andrew, the <= 9.2 failure https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2018-11-27%2018%3A18%3A21 seems to be related to nabstime etc not being available anymore. We've not backpatched Author: Tom Lane <tgl@sss.pgh.pa.us> Branch: REL_11_STABLE Release: REL_11_0 [184951a48] 2018-10-12 19:33:56 -0400 Branch: REL_10_STABLE Release: REL_10_6 [9320263ae] 2018-10-12 19:33:56 -0400 Branch: REL9_6_STABLE Release: REL9_6_11 [2ad422ce1] 2018-10-12 19:33:56 -0400 Branch: REL9_5_STABLE Release: REL9_5_15 [43cc4e49e] 2018-10-12 19:33:56 -0400 Branch: REL9_4_STABLE Release: REL9_4_20 [7b88c1ddd] 2018-10-12 19:33:57 -0400 Branch: REL9_3_STABLE Release: REL9_3_25 [fb583c80d] 2018-10-12 19:33:57 -0400 Remove abstime, reltime, tinterval tables from old regression databases. that far. I've locally verified that upgrade works after SELECT format('ALTER TABLE %s DROP COLUMN %s;', attrelid::regclass, attname) FROM pg_attribute WHERE attrelid > 16384 ANDatttypid IN ('abstime'::regtype, 'reltime'::regtype, 'tinterval'::regtype) \gexec or something in that vein. Greetings, Andres Freund
On 11/27/18 3:38 PM, Andres Freund wrote: > Hi, > > On 2018-11-27 10:47:29 -0500, Tom Lane wrote: >> Andrew Dunstan <andrew.dunstan@2ndquadrant.com> writes: >>> In any case, I think we will probably need to bite the bullet and have >>> pg_dump render LOs in OID order. >> +1. Looking at the relevant code: >> >> /* >> * Currently, we re-fetch all BLOB OIDs using a cursor. Consider scanning >> * the already-in-memory dumpable objects instead... >> */ >> if (fout->remoteVersion >= 90000) >> blobQry = "DECLARE bloboid CURSOR FOR SELECT oid FROM pg_largeobject_metadata"; >> else >> blobQry = "DECLARE bloboid CURSOR FOR SELECT DISTINCT loid FROM pg_largeobject"; > Done. Thanks for chiming in. > > Andrew, the <= 9.2 failure https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2018-11-27%2018%3A18%3A21 > seems to be related to nabstime etc not being available anymore. We've > not backpatched > > Author: Tom Lane <tgl@sss.pgh.pa.us> > Branch: REL_11_STABLE Release: REL_11_0 [184951a48] 2018-10-12 19:33:56 -0400 > Branch: REL_10_STABLE Release: REL_10_6 [9320263ae] 2018-10-12 19:33:56 -0400 > Branch: REL9_6_STABLE Release: REL9_6_11 [2ad422ce1] 2018-10-12 19:33:56 -0400 > Branch: REL9_5_STABLE Release: REL9_5_15 [43cc4e49e] 2018-10-12 19:33:56 -0400 > Branch: REL9_4_STABLE Release: REL9_4_20 [7b88c1ddd] 2018-10-12 19:33:57 -0400 > Branch: REL9_3_STABLE Release: REL9_3_25 [fb583c80d] 2018-10-12 19:33:57 -0400 > > Remove abstime, reltime, tinterval tables from old regression databases. > > that far. I've locally verified that upgrade works after > > SELECT format('ALTER TABLE %s DROP COLUMN %s;', attrelid::regclass, attname) FROM pg_attribute WHERE attrelid > 16384 ANDatttypid IN ('abstime'::regtype, 'reltime'::regtype, 'tinterval'::regtype) > \gexec > > or something in that vein. > Thanks. I'm just dropping the tables for REL_12. There is no point in just dropping the columns - they are one-column tables. The reason you're seeing this is that given the discussion about pg_upgrade support I wanted to make sure that we had coverage for live + 2 releases. Should be green the next time it reports. cheers andrew -- Andrew Dunstan https://www.2ndQuadrant.com PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services