[BUG] pg_upgrade test fails from older versions.

"Anton A. Melnikov" <aamelnikov@inbox.ru> writes:

2) In 60684dd83 and b5d63824 there are two changes in the set of specific privileges.
The thing is that in the privileges.sql test there is REVOKE DELETE command
which becomes pair of REVOKE ALL and GRANT all specific privileges except DELETE
in the result dump. Therefore, any change in the set of specific privileges will lead to
a non-zero dumps diff.
To avoid this, i propose to replace any specific GRANT and REVOKE in the result dumps with ALL.
This also made in the patch attached.

Isn't that likely to mask actual bugs?

regards, tom lane

On Mon, Dec 19, 2022 at 03:50:19AM +0300, Anton A. Melnikov wrote:

+-- The internal format of "aclitem" changed in PostgreSQL version 16
+-- so replace it with text type
+\if :oldpgversion_le15
+DO $$
+DECLARE
+    change_aclitem_type TEXT;
+BEGIN
+    FOR change_aclitem_type IN
+        SELECT 'ALTER TABLE ' || table_schema || '.' ||
+        table_name || ' ALTER COLUMN ' ||
+		column_name || ' SET DATA TYPE text;'
+        AS change_aclitem_type
+        FROM information_schema.columns
+        WHERE data_type = 'aclitem' and table_schema != 'pg_catalog'
+    LOOP
+        EXECUTE change_aclitem_type;
+    END LOOP;
+END;
+$$;
+\endif

This is forgetting about materialized views, which is something that
pg_upgrade would also complain about when checking for relations with
aclitems. As far as I can see, the only place in the main regression
test suite where we have an aclitem attribute is tab_core_types for
HEAD and the stable branches, so it would be enough to do this
change. Anyway, wouldn't it be better to use the same conditions as
the WITH OIDS queries a few lines above, at least for consistency?

Note that check_for_data_types_usage() checks for tables, matviews and
indexes.
--
Michael

On Sun, Dec 18, 2022 at 08:56:48PM -0500, Tom Lane wrote:

"Anton A. Melnikov" <aamelnikov@inbox.ru> writes:

2) In 60684dd83 and b5d63824 there are two changes in the set of specific privileges.
The thing is that in the privileges.sql test there is REVOKE DELETE command
which becomes pair of REVOKE ALL and GRANT all specific privileges except DELETE
in the result dump. Therefore, any change in the set of specific privileges will lead to
a non-zero dumps diff.
To avoid this, i propose to replace any specific GRANT and REVOKE in the result dumps with ALL.
This also made in the patch attached.

Isn't that likely to mask actual bugs?

+   # Replace specific privilegies with ALL
+   $dump_contents =~ s/^(GRANT\s|REVOKE\s)(\S*)\s/$1ALL /mgx;
Yes, this would silence some diffs in the dumps taken from the old and
the new clusters.  It seems to me that it is one of the things where
the original dumps have better be tweaked, as this does not cause a
hard failure when running pg_upgrade.

While thinking about that, an extra idea popped in my mind as it may
be interesting to be able to filter out some of the diffs in some
contexts. So what about adding in 002_pg_upgrade.pl a small-ish hook
in the shape of a new environment variable pointing to a file adds
some custom filtering rules?
--
Michael

Hello!

Divided patch into two parts: first part refers to the modification of
the old dump while the second one relates to dump filtering.

1) v2-0001-Remove-aclitem-from-old-dump.patch

On 19.12.2022 06:10, Michael Paquier wrote:

This is forgetting about materialized views, which is something that
pg_upgrade would also complain about when checking for relations with
aclitems. As far as I can see, the only place in the main regression
test suite where we have an aclitem attribute is tab_core_types for
HEAD and the stable branches, so it would be enough to do this
change. Anyway, wouldn't it be better to use the same conditions as
the WITH OIDS queries a few lines above, at least for consistency?

Note that check_for_data_types_usage() checks for tables, matviews and
indexes.

Found that 'ALTER ... ALTER COLUMN SET DATA TYPE text'
is not applicable to materialized views and indexes as well as DROP COLUMN.
So couldn't make anything better than drop its in the old dump if they
contain at least one column of 'aclitem' type.

i've tested this script with:
CREATE TABLE acltable AS SELECT 1 AS int, 'postgres=a/postgres'::aclitem AS aclitem;
CREATE MATERIALIZED VIEW aclmview AS SELECT 'postgres=a/postgres'::aclitem AS aclitem;
CREATE INDEX aclindex on acltable (int) INCLUDE (aclitem);
performed in the regression database before creating the old dump.

The only thing i haven't been able to find a case when an an 'acltype' column would
be preserved in the index when this type was replaced in the parent table.
So passing relkind = 'i' is probably redundant.
If it is possible to find such a case, it would be very interesting.

Also made the replacement logic for 'acltype' in the tables more closer
to above the script that removes OIDs columns. In this script found likewise that
ALTER TABLE ... SET WITHOUT OIDS is not applicable to materialized views
and ALTER MATERIALIZED VIEW doesn't support WITHOUT OIDS clause.
Besides i couldn't find any legal way to create materialized view with oids in versions 11 or lower.
Command 'CREATE MATERIALIZED VIEW' doesn't support WITH OIDS or (OIDS) clause,
as well as ALTER MATERIALIZED VIEW as mentioned above.
Even with GUC default_with_oids = true":
CREATE TABLE oidtable AS SELECT 1 AS int;
CREATE MATERIALIZED VIEW oidmv AS SELECT * FROM oidtable;
give:
postgres=# SELECT oid::regclass::text FROM pg_class WHERE relname !~ '^pg_' AND relhasoids;
oid
----------
oidtable
(1 row)
So suggest to exclude the check of materialized views from this DO block.
Would be grateful for remarks if i didn't consider some cases.

2) v2-0002-Additional-dumps-filtering.patch

On 19.12.2022 06:16, Michael Paquier wrote:

While thinking about that, an extra idea popped in my mind as it may
be interesting to be able to filter out some of the diffs in some
contexts. So what about adding in 002_pg_upgrade.pl a small-ish hook
in the shape of a new environment variable pointing to a file adds
some custom filtering rules?

Yes. Made a hook that allows to proceed an external text file with additional
filtering rules and example of such file. Please take a look on it.

With the best wishes,

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Thu, Dec 22, 2022 at 09:59:18AM +0300, Anton A. Melnikov wrote:

2) v2-0002-Additional-dumps-filtering.patch

+       # Replace specific privilegies with ALL
+       $dump_contents =~ s/^(GRANT\s|REVOKE\s)(\S*)\s/$1ALL /mgx;
This should not be in 0002, I guess..

Yes. Made a hook that allows to proceed an external text file with additional
filtering rules and example of such file. Please take a look on it.

With the best wishes,

Hmm. 0001 does a direct check on aclitem as data type used in an
attribute, but misses anything related to arrays, domains or even
composite types, not to mention that we'd miss uses of aclitems in
index expressions.

That's basically the kind of thing check_for_data_types_usage() does.
I am not sure that it is a good idea to provide a limited coverage if
we do that for matviews and indexes, and the complexity induced in
upgrade_adapt.sql is not really appealing either. For now, I have
fixed the most pressing part for tables to match with the buildfarm
code that just drops the aclitem column rather than doing that for all
the relations that could have one.

The part on WITH OIDS has been addressed in its own commit down to
v12, removing the handling for matviews but adding one for foreign
tables where the operation is supported.
--
Michael

On Fri, Dec 23, 2022 at 11:42:39AM +0900, Michael Paquier wrote:

Hmm. 0001 does a direct check on aclitem as data type used in an
attribute,

For now, I have fixed the most pressing part for tables to match with
the buildfarm

+DO $$
+  DECLARE
+    rec text;
+       col text;
+  BEGIN
+  FOR rec in
+    SELECT oid::regclass::text
+    FROM pg_class
+    WHERE relname !~ '^pg_'
+      AND relkind IN ('r')
+    ORDER BY 1
+  LOOP
+    FOR col in SELECT attname FROM pg_attribute
+      WHERE attrelid::regclass::text = rec
+      AND atttypid = 'aclitem'::regtype
+    LOOP
+      EXECUTE 'ALTER TABLE ' || quote_ident(rec) || ' ALTER COLUMN ' ||
+        quote_ident(col) || ' SET DATA TYPE text';
+    END LOOP;
+  END LOOP;
+  END; $$;

This will do a seq scan around pg_attribute for each relation (currently
~600)...

Here, that takes a few seconds in a debug build, and I guess it'll be
more painful when running under valgrind/discard_caches/antiquated
hardware/etc.

This would do a single seqscan:
SELECT format('ALTER TABLE %I ALTER COLUMN %I TYPE TEXT', attrelid::regclass, attname) FROM pg_attribute WHERE atttypid='aclitem'::regtype; -- AND ...
\gexec

--
Justin

On Thu, Dec 22, 2022 at 09:27:24PM -0600, Justin Pryzby wrote:

This would do a single seqscan:
SELECT format('ALTER TABLE %I ALTER COLUMN %I TYPE TEXT',
attrelid::regclass, attname) FROM pg_attribute WHERE
atttypid='aclitem'::regtype; -- AND ...
\gexec

FWIW, I find the use of a FOR loop with a DO block much cleaner to
follow in this context, so something like the attached would be able
to group the two queries and address your point on O(N^2). Do you
like that?
--
Michael

Hello!

On 23.12.2022 06:27, Justin Pryzby wrote:

This would do a single seqscan:
SELECT format('ALTER TABLE %I ALTER COLUMN %I TYPE TEXT', attrelid::regclass, attname) FROM pg_attribute WHERE atttypid='aclitem'::regtype; -- AND ...
\gexec

Touched a bit on how long it takes to execute different types of queries on my PC.
At each measurement, the server restarted with a freshly copied regression database.
1)
DO $$
DECLARE
change_aclitem_type TEXT;
BEGIN
FOR change_aclitem_type IN
SELECT 'ALTER TABLE ' || table_schema || '.' ||
table_name || ' ALTER COLUMN ' ||
column_name || ' SET DATA TYPE text;'
AS change_aclitem_type
FROM information_schema.columns
WHERE data_type = 'aclitem' and table_schema != 'pg_catalog'
LOOP
EXECUTE change_aclitem_type;
END LOOP;
END;
$$;

2)
DO $$
DECLARE
rec text;
col text;
BEGIN
FOR rec in
SELECT oid::regclass::text
FROM pg_class
WHERE relname !~ '^pg_'
AND relkind IN ('r')
ORDER BY 1
LOOP
FOR col in SELECT attname FROM pg_attribute
WHERE attrelid::regclass::text = rec
AND atttypid = 'aclitem'::regtype
LOOP
EXECUTE 'ALTER TABLE ' || quote_ident(rec) || ' ALTER COLUMN ' ||
quote_ident(col) || ' SET DATA TYPE text';
END LOOP;
END LOOP;
END; $$;

3)
SELECT format('ALTER TABLE %I ALTER COLUMN %I TYPE TEXT', attrelid::regclass, attname) FROM pg_attribute WHERE atttypid='aclitem'::regtype;
\gexec

4) The same as 3) but in the DO block
DO $$
DECLARE
change_aclitem_type TEXT;
BEGIN
FOR change_aclitem_type IN
SELECT 'ALTER TABLE ' || attrelid::regclass || ' ALTER COLUMN ' ||
attname || ' TYPE TEXT;'
AS change_aclitem_type
FROM pg_attribute
WHERE atttypid = 'aclitem'::regtype
LOOP
EXECUTE change_aclitem_type;
END LOOP;
END;
$$;

Average execution time for three times:
_____________________________________
|N of query: | 1 | 2 | 3 | 4 |
|____________________________________
|Avg time, ms: | 58 | 1076 | 51 | 33 |
|____________________________________

Raw results in timing.txt

Best wishes,

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Sorry, didn't get to see the last letter!

On 23.12.2022 11:51, Michael Paquier wrote:

FWIW, I find the use of a FOR loop with a DO block much cleaner to
follow in this context, so something like the attached would be able
to group the two queries and address your point on O(N^2). Do you
like that?
--
Michael

The query:

DO $$
DECLARE
rec record;
BEGIN
FOR rec in
SELECT oid::regclass::text as rel, attname as col
FROM pg_class c, pg_attribute a
WHERE c.relname !~ '^pg_'
AND c.relkind IN ('r')
AND a.attrelid = c.oid
AND a.atttypid = 'aclitem'::regtype
ORDER BY 1
LOOP
EXECUTE 'ALTER TABLE ' || quote_ident(rec.rel) || ' ALTER COLUMN ' ||
quote_ident(rec.col) || ' SET DATA TYPE text';
END LOOP;
END; $$;

gives the average time of 36 ms at the same conditions.

With the best wishes!

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Fri, Dec 23, 2022 at 05:51:28PM +0900, Michael Paquier wrote:

On Thu, Dec 22, 2022 at 09:27:24PM -0600, Justin Pryzby wrote:

This would do a single seqscan:
SELECT format('ALTER TABLE %I ALTER COLUMN %I TYPE TEXT',
attrelid::regclass, attname) FROM pg_attribute WHERE
atttypid='aclitem'::regtype; -- AND ...
\gexec

FWIW, I find the use of a FOR loop with a DO block much cleaner to
follow in this context, so something like the attached would be able
to group the two queries and address your point on O(N^2). Do you
like that?

LGTM. Thanks.

--
Justin

On Fri, Dec 23, 2022 at 10:39:25AM -0600, Justin Pryzby wrote:

On Fri, Dec 23, 2022 at 05:51:28PM +0900, Michael Paquier wrote:

FWIW, I find the use of a FOR loop with a DO block much cleaner to
follow in this context, so something like the attached would be able
to group the two queries and address your point on O(N^2). Do you
like that?

LGTM. Thanks.

I am a bit busy for the next few days, but I may be able to get that
done next Monday.
--
Michael

On Fri, Dec 23, 2022 at 10:39:25AM -0600, Justin Pryzby wrote:

LGTM. Thanks.

Done as of d3c0cc4.
--
Michael

On Fri, Dec 23, 2022 at 12:43:00PM +0300, Anton A. Melnikov wrote:

Sorry, didn't get to see the last letter!

No worries, the result is the same :)

I was looking at 0002 to add a callback to provide custom filtering
rules.

+ my @ext_filter = split('\/', $_);
Are you sure that enforcing a separation with a slash is a good idea?
What if the filters include directory paths or characters that are
escaped, for example?

Rather than introducing a filter.regex, I would tend to just document
that in the README with a small example. I have been considering a
few alternatives while making this useful in most cases, still my mind
alrways comes back to the simplest thing we to just read each line of
the file, chomp it and apply the pattern to the log file..
--
Michael

Hello!

On 23.12.2022 05:42, Michael Paquier wrote:

On Thu, Dec 22, 2022 at 09:59:18AM +0300, Anton A. Melnikov wrote:

2) v2-0002-Additional-dumps-filtering.patch

+       # Replace specific privilegies with ALL
+       $dump_contents =~ s/^(GRANT\s|REVOKE\s)(\S*)\s/$1ALL /mgx;
This should not be in 0002, I guess..

Made a separate patch for it: v3-0001-Fix-dumps-filtering.patch

On 26.12.2022 05:52, Michael Paquier wrote:

On Fri, Dec 23, 2022 at 12:43:00PM +0300, Anton A. Melnikov wrote:
I was looking at 0002 to add a callback to provide custom filtering
rules.

+ my @ext_filter = split('\/', $_);
Are you sure that enforcing a separation with a slash is a good idea?
What if the filters include directory paths or characters that are
escaped, for example?

Rather than introducing a filter.regex, I would tend to just document
that in the README with a small example. I have been considering a
few alternatives while making this useful in most cases, still my mind
alrways comes back to the simplest thing we to just read each line of
the file, chomp it and apply the pattern to the log file..

Thanks for your attention!
Yes, indeed. It will be really simpler.
Made it in the v3-0002-Add-external-dumps-filtering.patch

With the best wishes,

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Mon, Dec 26, 2022 at 09:22:08AM +0300, Anton A. Melnikov wrote:

Made a separate patch for it: v3-0001-Fix-dumps-filtering.patch

Well, the thing about this part is that is it is not needed: the same
can be achieved with 0002 in place.

Yes, indeed. It will be really simpler.
Made it in the v3-0002-Add-external-dumps-filtering.patch

I have fixed a few things in the patch, like switching the step
skipping comments with a regexp, adding one step to ignore empty
lines, applying a proper indentation and fixing comments here and
there (TESTING was incorrect, btw).

It is worth noting that perlcritic was complaining here, as eval is
getting used with a string. I have spent a few days looking at that,
and I really want a maximum of flexibility in the rules that can be
applied so I have put a "no critic" rule, which is fine by me as this
extra file is something owned by the user and it would apply only to
cross-version upgrades.

So it looks like we are now done here.. With all these pieces in
place in the tests, I don't see why it would not be possible to
automate the cross-version tests of pg_upgrade.
--
Michael

Hello!

On 27.12.2022 08:44, Michael Paquier wrote:

It is worth noting that perlcritic was complaining here, as eval is
getting used with a string. I have spent a few days looking at that,
and I really want a maximum of flexibility in the rules that can be
applied so I have put a "no critic" rule, which is fine by me as this
extra file is something owned by the user and it would apply only to
cross-version upgrades.

I think it's a very smart decision. Thank you very match!

So it looks like we are now done here.. With all these pieces in
place in the tests, I don't see why it would not be possible to
automate the cross-version tests of pg_upgrade.

I've checked the cross-upgrade test form 9.5+ to current master and
have found no problem with accuracy up to dumps filtering.
For cross-version tests automation one have to write additional
filtering rules in the external files.

I would like to try realize this, better in a separate thread.
If there are no other considerations could you close the corresponding
record on the January CF, please?

With the best wishes!

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

On Tue, Dec 27, 2022 at 03:26:10PM +0300, Anton A. Melnikov wrote:

I would like to try realize this, better in a separate thread.

I don't think that this should be added into the tree, but if you have
per-version filtering rules, of course feel free to publish that to
the lists. I am sure that this could be helpful for others.

If there are no other considerations could you close the corresponding
record on the January CF, please?

Indeed, now marked as committed.
--
Michael

On 27.12.2022 16:50, Michael Paquier wrote:

If there are no other considerations could you close the corresponding
record on the January CF, please?

Indeed, now marked as committed.

-

Thanks a lot!

Merry Christmas!

--
Anton A. Melnikov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company