ALTER TABLE lock downgrades have broken pg_upgrade
There is logic in pg_upgrade plus the backend, mostly added by commit
4c6780fd1, to cope with the corner cases that sometimes arise where the
old and new versions have different ideas about whether a given table
needs a TOAST table. The more common case is where there's a TOAST table
in the old DB, but (perhaps as a result of having dropped all the wide
columns) the new cluster doesn't think the table definition requires a
TOAST table. The reverse is also possible, although according to the
existing code comments it can only happen when upgrading from pre-9.1.
The way pg_upgrade handles that case is that after running all the
table creation operations it issues this command:
PQclear(executeQueryOrDie(conn, "ALTER TABLE %s.%s RESET (binary_upgrade_dummy_option);",
quote_identifier(PQgetvalue(res, rowno, i_nspname)),
quote_identifier(PQgetvalue(res, rowno, i_relname))));
which doesn't actually do anything (no such reloption being set) but
nonetheless triggers a call of AlterTableCreateToastTable, which
will cause a toast table to be created if the new server thinks the
table definition requires one.
Or at least, it did until Simon decided that ALTER TABLE RESET
doesn't require AccessExclusiveLock. Now you get a failure.
I haven't tried to construct a pre-9.1 database that would trigger
this, but you can make it happen by applying the attached patch
to create a toast-table-less table in the regression tests,
and then doing "make check" in src/bin/pg_upgrade. You get this:
...
Restoring database schemas in the new cluster
ok
Creating newly-required TOAST tables SQL command failed
ALTER TABLE "public"."i_once_had_a_toast_table" RESET (binary_upgrade_dummy_option);
ERROR: AccessExclusiveLock required to add toast table.
Failure, exiting
+ rm -rf /tmp/pg_upgrade_check-o0CUMm
make: *** [check] Error 1
I think possibly the easiest fix for this is to have pg_upgrade,
instead of RESETting a nonexistent option, RESET something that's
still considered to require AccessExclusiveLock. "user_catalog_table"
would work, looks like; though I'd want to annotate its entry in
reloptions.c to warn people away from downgrading its lock level.
More generally, though, I wonder how we can have some test coverage
on such cases going forward. Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?
regards, tom lane
Attachments:
fake-toast-less-table.patchtext/x-diff; charset=us-ascii; name=fake-toast-less-table.patchDownload
diff --git a/src/test/regress/expected/indirect_toast.out b/src/test/regress/expected/indirect_toast.out
index 4f4bf41..ad7127d 100644
*** a/src/test/regress/expected/indirect_toast.out
--- b/src/test/regress/expected/indirect_toast.out
*************** SELECT substring(toasttest::text, 1, 200
*** 149,151 ****
--- 149,158 ----
DROP TABLE toasttest;
DROP FUNCTION update_using_indirect();
+ -- Create a table that has a toast table, then modify it so it appears
+ -- not to have one, and leave it behind after the regression tests end.
+ -- This enables testing of this scenario for pg_upgrade.
+ create table i_once_had_a_toast_table(f1 int, f2 text);
+ insert into i_once_had_a_toast_table values(1, 'foo');
+ update pg_class set reltoastrelid = 0
+ where relname = 'i_once_had_a_toast_table';
diff --git a/src/test/regress/sql/indirect_toast.sql b/src/test/regress/sql/indirect_toast.sql
index d502480..cefbd0b 100644
*** a/src/test/regress/sql/indirect_toast.sql
--- b/src/test/regress/sql/indirect_toast.sql
*************** SELECT substring(toasttest::text, 1, 200
*** 59,61 ****
--- 59,69 ----
DROP TABLE toasttest;
DROP FUNCTION update_using_indirect();
+
+ -- Create a table that has a toast table, then modify it so it appears
+ -- not to have one, and leave it behind after the regression tests end.
+ -- This enables testing of this scenario for pg_upgrade.
+ create table i_once_had_a_toast_table(f1 int, f2 text);
+ insert into i_once_had_a_toast_table values(1, 'foo');
+ update pg_class set reltoastrelid = 0
+ where relname = 'i_once_had_a_toast_table';
Tom Lane wrote:
More generally, though, I wonder how we can have some test coverage
on such cases going forward. Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?
I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the latest
stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
Tom Lane wrote:
More generally, though, I wonder how we can have some test coverage
on such cases going forward. Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the latest
stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.
That would help greatly with pg_dump test coverage as well.. One of the
problems of trying to get good LOC coverage of pg_dump is that a *lot*
of the code is version-specific...
Thanks!
Stephen
Stephen Frost wrote:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
Tom Lane wrote:
More generally, though, I wonder how we can have some test coverage
on such cases going forward. Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the latest
stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.That would help greatly with pg_dump test coverage as well.. One of the
problems of trying to get good LOC coverage of pg_dump is that a *lot*
of the code is version-specific...
If we can put together a script that runs test.sh for various versions
and then verifies the runs, we could use it in both buildfarm and
coverage.
--
�lvaro Herrera http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 05/03/2016 01:21 PM, Stephen Frost wrote:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
Tom Lane wrote:
More generally, though, I wonder how we can have some test coverage
on such cases going forward. Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the latest
stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.That would help greatly with pg_dump test coverage as well.. One of the
problems of trying to get good LOC coverage of pg_dump is that a *lot*
of the code is version-specific...
I have an module that does it, although it's not really stable enough.
But it's a big start.
See
<https://github.com/PGBuildFarm/client-code/blob/master/PGBuild/Modules/TestUpgradeXversion.pm>
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
Stephen Frost wrote:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
Tom Lane wrote:
More generally, though, I wonder how we can have some test coverage
on such cases going forward. Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the latest
stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.That would help greatly with pg_dump test coverage as well.. One of the
problems of trying to get good LOC coverage of pg_dump is that a *lot*
of the code is version-specific...If we can put together a script that runs test.sh for various versions
and then verifies the runs, we could use it in both buildfarm and
coverage.
One other point is that pg_dump goes quite a bit farther back than just
what we currently support (or at least, it tries to). I think that,
generally, that's a good thing, but it does mean we have a lot of cases
that don't get tested nearly as much...
I was able to get back to 7.4 up and running without too much trouble,
but even that doesn't cover all the cases we have in pg_dump. I'm not
sure if we want to define a "we will support pg_dump back to X" cut-off
or if we want to try and get older versions to run on modern systems,
but it's definitely worth pointing out that we're trying to support much
farther back than what is currently supported in pg_dump today.
Thanks!
Stephen
On 05/03/2016 01:28 PM, Andrew Dunstan wrote:
On 05/03/2016 01:21 PM, Stephen Frost wrote:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
Tom Lane wrote:
More generally, though, I wonder how we can have some test coverage
on such cases going forward. Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the
latest
stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.That would help greatly with pg_dump test coverage as well.. One of the
problems of trying to get good LOC coverage of pg_dump is that a *lot*
of the code is version-specific...I have an module that does it, although it's not really stable enough.
But it's a big start.
See
<https://github.com/PGBuildFarm/client-code/blob/master/PGBuild/Modules/TestUpgradeXversion.pm>
Incidentally, just as a warning for anyone trying, this uses up a quite
a lot of disk space.
You would need several GB available.
cheers
andrew
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 05/03/2016 01:33 PM, Andrew Dunstan wrote:
On 05/03/2016 01:28 PM, Andrew Dunstan wrote:
On 05/03/2016 01:21 PM, Stephen Frost wrote:
* Alvaro Herrera (alvherre@2ndquadrant.com) wrote:
Tom Lane wrote:
More generally, though, I wonder how we can have some test coverage
on such cases going forward. Is the patch below too ugly to commit
permanently, and if so, what other idea can you suggest?I suggest a buildfarm animal running a custom buildfarm module that
exercises the pg_upgrade test from every supported version to the
latest
stable and to master -- together with your proposed case that leaves a
toastless table around for pg_upgrade to handle.That would help greatly with pg_dump test coverage as well.. One of the
problems of trying to get good LOC coverage of pg_dump is that a *lot*
of the code is version-specific...I have an module that does it, although it's not really stable
enough. But it's a big start.
See
<https://github.com/PGBuildFarm/client-code/blob/master/PGBuild/Modules/TestUpgradeXversion.pm>Incidentally, just as a warning for anyone trying, this uses up a
quite a lot of disk space.You would need several GB available.
And if this is of any use, here are the dump differences from every live
version to git tip, as of this morning.
cheers
andrew
Attachments:
upgrade_dump_diff.tgzapplication/x-tar; name=upgrade_dump_diff.tgzDownload
�
�(W �]ks������Wt��5�1`��8�����8�k���[�� �T !��v?$�����L(��Xju����w�Z-�m������}������j�{}�R�G�������G�JXQ�`��2VU�'V�,=AR�f���rzb�f��5�^����n������O&���=q�;��o\kf������8��f������x*KXoKZ[R���t*����E6��TIzv||\$���l��gQm�i��i����u$S��JW����-�-��u����P��=Cg�������@n���@���'c��q��f�����wh�����|�<{�^X�-����%�L�����r��������G��4��}{9|�>||}q~V����v����
S��
����E<^�.?���[]I�l���>�c��x�il��%�����������Bv���t���6�8k</��T�����5���������
xc_[��G�� Q7��z���@��Mc ���� B�j�4�������vi��ifX+��!���n\.p���q��-�I+}Wn ������g�W�{�f�gM���f0��dH��8�g��u�aH��nwd�����>���U�kNvh}3&�R�=�����W�����GI�H���������C����ki�����9��p�;�x������:E��h�ue��!����'&�Z+{���M����R�:�8�Q.\������F��I�)��a�u�\)��k?�j�qp6:��t$�������Z�(R����z�6������p�t�}����-��c8�g2�� m����.���)'�0��h{6(C��"�a�L�+�L0e�{`B&�2Ai� ����T�u�4\���N!$ �u��`CWV8�.>���������}`�"a�S6(����P�
��b���$����&?�
X�l��:@������d���`�&���������P$K�
�6Ta��`�Y�� 4����HQ�
)�=Y~�����U|`C%6`Mfl���/l�T�
�|`C56g����`�O������JlP��>t�^�>)�)�Y���Yd%6���f��n��,R�����0O)������
���)M��H�v�a���M2�H]j?F
�<y�l0��HQ�
�������d�0%��0�*������aj���?�2�8�yp�����.���=qo��x�����v���
���Dw R"a�=�2��~<�2�������0;��>Q��������P���>����;k!8�X�
�X�k��f�`��_�'���x��������h2��k
-�l��>m���5v�vE�V�����H�l<;~��"����L��O������Kv�`� ������VS} �����d��@�59&u����j���k�#z��a���XmZ��Eq��,��q�8hb�����Sr�3j��?��N�so��T�-�
�K*��M^�'-F�QZ�A��w�^h(|y���k3����5S���5��j�
���P�
&f�M���_�H�}6���
��@S���=��l����7��rtv�^c��������������ws�>�������u�bg�����h�e�����p>���,E�,����RlL��3�
��]M9�h������������ktM�
�U��l0q��a?V�T���s��X�|��G��gC�i��}k��|/����pp5����n
WS��t� ���&|��+����
j���W0�������5_����)}'�W��^ ������B��d�D�#�S��
hHF
L���1 ���F I�U��F�7���WM�ef�3g�P�L�>:=�3�������0�e7G=�Qb��
��T�k� $89���}?�D7�����9�n�����hh��7^��b����$j��v~s����T��/���w��>���K ��[��-��em Q�aB�Q������L�5���K���%2q�y���v!�L��X�M�m������w�������y]�f��6��H5���m�r��%d7�m|�~A�����8Jvd^1��_V��$��8�X�hch��13���,ie�H�.b7f����J������������E
����s{1�a�/��;�k��p�p<~B�k����JAS�@{ ������������[�Yhu�'�����h/�b����Ya?MKt�������EB������AD���<��Z������p �y� ������&��"sWI�&D�n�;����z-�yg3�fC�u�����tR6�����h)r���]�=�RO��R���1�s}�B�q��b�P����dK%�`k�k�z���e��(�@��(;X�{�R �K�W�:������
�po'��:�
5E��yv�$W-�D��,�L�$M/�Z��9�#����[X*]NvP�#���4%�p��]����D�q��� ���"g�K����a�R�B����!������;�����h�v5
�wl�.��u*�8��1���7��o�����O��,��Q����� �K���0��<��<�egY+�����]<��������*y QV�-5�j9b������6I7��-B����"6��'c����d���������^�<A8�q{��vL�_6r�+0t���{�=KmG����������c��G-D��+��a��\&�/
�}dA
K��Bw�OUJ�-J`�G$�@@�@\�����x�4��(�9%Kty��O�Wj9�ZX�(���1g9�IE�}R�c�zD�%$���<cNQ���i@��$��"0�cZ)D��+�R���Z
�h�I1�^���� '����
S*5'�c��f�H��� 7�'��"�5?����]�,���v������s<���M��7h��lr 4@��v`~��+p������(������#�7�K�d2!����N�B ?W�( ���0����)��|yY��e3?�����!O���($�$���`\�X&
XB�UI���p�����_���i4��E�Z�������}G�u��h��+�n�\�fq�S+��0�mP
��<��2/Nf"i�C<�'�����T��Z��,����\�q1r-F���;#W�4�7J��0��f���&}��%���
i�L��s9��) ��~���� �l����!��cU����x�mG����>��v�aR.>!��f��@�{;4���v�-YV�3���37����
!��&�R�L�I�����|�CW���Y+�������p��#G�
?X8S�e�0��8�B�6��rwt��V�����/\�
D2�}��Y �Os5��R-��,��ld�g1�[0���/���%d�%y��l�[�����pM\](� �C�=���B.�"�����g��Yo�l����FA����e������+�O�Gi��]���a��)��)1���}0jz��2��E.�F����G��&�V�:+��'�~fo�v���:GP@�A������b`|o]Y��-�����e>��r�\���t&�:�vx��5��LL�� �G�l��-{�;J���$�2�E�#@&������S�!��wy�oB'������L_��8!xz@W@�0$�{>����k���1�!/���3S�1�1����_;N�m�#5l��T�,?�<?���Q����r8�D��:y�g����W�e�������o�p?����S�*����B��:�ea`�a �����=;`�U���wq� og����#n-��zp��%�9=���olwn-��"��N��02�0�o��n!F�O
]�P9�GI�����/�5�K_��#E�on��������s*JK��[���l�j�N��B����%��������8��?i�F�!������k��b~���_�B�b&�d���t��%����M�E�������#6f\�XQ+��z�]�v�I���R[��I�)�G��:=���u�������?�z�[�$�}]6�zYH� Q�������n�����`\�������p�q� ��.F�Q��&�XGFM��&!o0�G����!4�-V[�W�MDu
��-����3{�x(#�8�o`f�
��zgp�2�?�����]9~��I�0�����?���dk��������z�����5��@��H��%l�b�����"w$T�7�sG]���|��3� .���<I8�0��`�����,;�����}_�G��]��<�K��Zq�b�����)�*f8+'�2 ���2,UCr��y�;�E������x2vq]���e$���Ll���cz�J���H��Xq�G�����R�����b1�Q^R�fH���*q���������\H��9J��*y��b��<��2��aC%>{�XG��'#��|��xBJ5���x�����p����F]��J|t���b������� ������o Zh��>�.�5�}Fm���7��r�����
���]*�����'=(�B*Mk��J�k����2A�.��S`�W�2��B��gP����F_���s�x�Tps�����zQ%���O��� GF�%f�^M�L�n�>w�d^%�I��'!
��"��v7�J8�sY����.�uD�����5[�������>�I������\��+Y��,��VE6���U_^�2D� �"bK���w�p�?��������n�����N�sq����f�a����M������gG����,�!���3�G)v�'�V���� 3�TaB�}�����n��$����D���YEzd������^/r���"Rjd��d-���^..cZZ�/�%�"�V}=&4,sz��\L�3�3���v���U���\ =�;�?4&�7�Y1��,����B�|��eEB�-"�e�n<