Horton

Thread

Commits

  1. Add custom filtering rules to the TAP tests of pg_upgrade

  2. Switch query fixing aclitems in ~15 from O(N^2) to O(N) in upgrade_adapt.sql

  3. Update upgrade_adapt.sql to handle tables using aclitem as data type

  4. Fix some incorrectness in upgrade_adapt.sql on query for WITH OIDS

  1. [BUG] pg_upgrade test fails from older versions.

    Anton A. Melnikov <aamelnikov@inbox.ru> — 2022-12-19T00:50:19Z

    Hello!

Found that pg_upgrade test has broken for upgrades from older versions.
This happened for two reasons.
1) In 7b378237a the format of "aclitem" changed so upgrade from <=15
fails with error:
"Your installation contains the "aclitem" data type in user tables.
The internal format of "aclitem" changed in PostgreSQL version 16
so this cluster cannot currently be upgraded... "

Tried to fix it by changing the column type in the upgrade_adapt.sql.
Please see the patch attached.

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.

Would be glad to any remarks.

With best regards,

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

  2. Re: [BUG] pg_upgrade test fails from older versions.

    Tom Lane <tgl@sss.pgh.pa.us> — 2022-12-19T01:56:48Z

    "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

  3. Re: [BUG] pg_upgrade test fails from older versions.

    Michael Paquier <michael@paquier.xyz> — 2022-12-19T03:10:30Z

    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

  4. Re: [BUG] pg_upgrade test fails from older versions.

    Michael Paquier <michael@paquier.xyz> — 2022-12-19T03:16:18Z

    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

  5. Re: [BUG] pg_upgrade test fails from older versions.

    Anton A. Melnikov <aamelnikov@inbox.ru> — 2022-12-22T06:59:18Z

    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

  6. Re: [BUG] pg_upgrade test fails from older versions.

    Michael Paquier <michael@paquier.xyz> — 2022-12-23T02:42:39Z

    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

  7. Re: [BUG] pg_upgrade test fails from older versions.

    Justin Pryzby <pryzby@telsasoft.com> — 2022-12-23T03:27:24Z

    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

  8. Re: [BUG] pg_upgrade test fails from older versions.

    Michael Paquier <michael@paquier.xyz> — 2022-12-23T08:51:28Z

    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

  9. Re: [BUG] pg_upgrade test fails from older versions.

    Anton A. Melnikov <aamelnikov@inbox.ru> — 2022-12-23T09:17:18Z

    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

  10. Re: [BUG] pg_upgrade test fails from older versions.

    Anton A. Melnikov <aamelnikov@inbox.ru> — 2022-12-23T09:43:00Z

    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

  11. Re: [BUG] pg_upgrade test fails from older versions.

    Justin Pryzby <pryzby@telsasoft.com> — 2022-12-23T16:39:25Z

    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

  12. Re: [BUG] pg_upgrade test fails from older versions.

    Michael Paquier <michael@paquier.xyz> — 2022-12-24T00:55:07Z

    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

  13. Re: [BUG] pg_upgrade test fails from older versions.

    Michael Paquier <michael@paquier.xyz> — 2022-12-25T23:02:28Z

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

Done as of d3c0cc4.
--
Michael

  14. Re: [BUG] pg_upgrade test fails from older versions.

    Michael Paquier <michael@paquier.xyz> — 2022-12-26T02:52:29Z

    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

  15. Re: [BUG] pg_upgrade test fails from older versions.

    Anton A. Melnikov <aamelnikov@inbox.ru> — 2022-12-26T06:22:08Z

    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

  16. Re: [BUG] pg_upgrade test fails from older versions.

    Michael Paquier <michael@paquier.xyz> — 2022-12-27T05:44:38Z

    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

  17. Re: [BUG] pg_upgrade test fails from older versions.

    Anton A. Melnikov <aamelnikov@inbox.ru> — 2022-12-27T12:26:10Z

    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

  18. Re: [BUG] pg_upgrade test fails from older versions.

    Michael Paquier <michael@paquier.xyz> — 2022-12-27T13:50:14Z

    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

  19. Re: [BUG] pg_upgrade test fails from older versions.

    Anton A. Melnikov <aamelnikov@inbox.ru> — 2022-12-27T13:55:54Z

    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