ALTER TABLE lock downgrades have broken pg_upgrade

Started by Tom Laneover 9 years ago27 messages
#1Tom Lane
tgl@sss.pgh.pa.us
1 attachment(s)

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';
#2Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Tom Lane (#1)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

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

#3Stephen Frost
sfrost@snowman.net
In reply to: Alvaro Herrera (#2)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

* 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

#4Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Stephen Frost (#3)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

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

#5Andrew Dunstan
andrew@dunslane.net
In reply to: Stephen Frost (#3)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

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&gt;

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

#6Stephen Frost
sfrost@snowman.net
In reply to: Alvaro Herrera (#4)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

* 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

#7Andrew Dunstan
andrew@dunslane.net
In reply to: Andrew Dunstan (#5)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

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&gt;

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

#8Andrew Dunstan
andrew@dunslane.net
In reply to: Andrew Dunstan (#7)
1 attachment(s)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

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&gt;

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���FI�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��H5���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�	������&��"sWI�&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 �Os5��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<u�������I�F�Y�����V��<���o�V�r����3&��~���:'�LZ���.����;y8:���������m�.s����41������F�2i��+Xr+4P�I���e�@�#�X���I���F�L��2�����x�'`g�<3lYC����[��������-����X�*7u�V�"�R��Y����������|����C���������(��4�9%/���(i'	#���$�6����
Z���h]q������o��#c�a��zJ��x��������Z�#o���*Of�`k�[����(-1tfLR�G
�����"�.�#R&i�d���Vp�5e��%���i6���
��������k��%�R�������~�`�G)9(5vu�UruY���0v>��5��	�dQ��`��S��{@�p��1��|�Z+`C�w�������9����x�S:u��e�s-�WQZ�k9�c��X����>���Dx!�C�b���������9iN���$�<H��{���59���f��k�����'���3�%`��Q�O����m6��v]t������[�	>������+a��x�e������D�Ez��z�H�l���=�;7���]����!����Y�'�k����W�O�����'�`p?q���|%Hg�e�T������� �O5|�j�k���*���J�����"�Sgi��gQm�i��i��S��
&����*��e����������0C���]��2w>�u�b:�q>z�F��.H���#���� ���_FRf4�����������d��������
����E<^�.?���[]I�lB( ,��FR���P"��H\/DadT^L�pL7��K� ��K�ck2q��9���
���������n"T%@��?.������N�{���TiXM)=�I1���v��"5�Y�
dE�7.�X����
����$���k��vW�s�-C��~��|�;��{���;�>��YkeO����6��	*Z&�Af��Q�o?��/��#�������B@���f��Q�kW%�{`������j�������������4����5�������qR��(�x��,�N����(����T"��o���)���F\}<��'��7d��i��9��=��	qn� ��	L���
A�& ��u��Q�Bv��W'�S��c�D��AH��I�e��l�"�0��GZ���d����� ��
Al@�����~�"��A��D ul��O`���[�����6`+�����'l`��@"��l��@=�~�:6��/�!�
�b�����l�"�!�
:60�6`&������� 6`����N�b�	\/�0{�Al ���N��I$��
\��al�DX6�����w�l\F6���3��������)��;����)�����+�`?�Sp����3�Al���1g�c
�g���
Al���a�$}a�pc
),%�8���z������
�^�����������j]�E"mE$d},*�a?�#	�TN�H��S.����}�a!��H+�i�j0A�GZ�h?>�
!�gC\��	�d���R?�#�w	����@60��@���Kr��QRX�����^N?|�,/���Tur���?O���_�5��r�K�Zu������b��y���469
�R�5L����!�H��0�����Rx6�Y�@6H(�1�D����	a�����!�������G���]|�~<��/����8?y��_M��p>��-h������@��C�}�����Px��4�%�Mg����{�c�"������.�R���� H�>Q���>�S@/����J%��pH��`���,�W���:
M������{������?WWZ�I_�l�5�/d������4�r���k*C�
!j��t�0]��1�9��J�d}3��o���������a����t�MH&�������F��D=��L/��R�z��?����S`������6-����@l�r�N���@o!k�r����1sr�������:�>,����?��{}�9�q�=��`����������]9�X	����?%�aiK�jAvV��waeY�8�5�;8��{�d����dn��r"�����.����%�)IkK�-�D�D�
��VUd���T%{����/���Y�\�������cK��x=������]i�������lz�K���~�^�* g�����������yu��"K����
]�����2UI!E�Y�����s�@W�l*���������b��M��}u��ztz5���m��JY����ls�����m����M������/�����d���zy?9D[f[;���9[Sh��le�{-i�C��9��N������&H�Y}��m�[>������y��4�sa��
����Kj���-X���������c�%7�yW3o.���Y�4�wU�w���j�=s��������+k����<�� [���Uc����i�R���������\��:��!n��Vcd;�m(����*���f�T��V"�
��6���z:�]i�9k�]�][���j�X�}��8��h����a�3�����W����O����beW��,�����O�����+[$+3c���I2��-��
��h��O]�����f���~trp�w
A�N����J^�$�d�����BM�r���|vT�X�����x5�8Q�����x�X������r|�O�	���_�=�W������c'�x�E��b|����!r���b��j��5!6c����cZG���uai��"��$Ev��|J�3{�.���|}�����R���P��qs�"��z�=T	����dbO����W�OJ�W�b��0��hz��B�tT-�&�����C�����Rz}Xy��k!�.��jy@(���x#m��������!n�{����a'u�]�T�L����ZTaV��@~0�1��x���O�_~NCo���j�n�*G�J�'C��II��
g�qh��N�-�������n�GH=P��)�Z�CH[ ��~Su'���U�'�<t��c}�y���u1�N���tn�=��T�}?W
��nm��~QM�o���\��g~��3�w�{���X�s�~�����~��L	���)�����m������l��|6���@ �w�Q�}��Qp��I�[�-�r_�T������'�<�r>F�nE��D2�!�e�&����)�si��s����H!8�|����"M��:Ke�F�>��K�ho��)/�1���%��l�E��	�m)��%�CHZly����l��X���j���]����5Y��f�����%se0Z/�#��&EH�4���^��
�!x'5��{?p�����#i)�>�"}���B�fY�K���$I�N����)R������:�i{�ji���BL�[S��Y������B/�)�bEU�����x+m��[�&���R��J���>{sQ����I���Z�+�7�2M%}�b������+�W%����������k���
 $u(T;������v�4V�MZ�����i�M����j���4�*?��X��D��ov~)���R��L�)���$�|��!`5�E��d���:nV���s
[k@�iz�C���,������p�M�q�����r2)�,�3��l���x�U*o�^��,��D�GU�
���1�����{=�:i\9T�J_��
��2���W�H���M�Va�yn��#WV9D�y�s�x�'7��O��>���bl�Q��������h6Z>�w4���f�����5�U���7�Jw��2TM,���6K��`iI����+��<���S���v���9�3[��6�[~va�*yHco�>rc.
s}�����o�^�\�Lf��J�zl�w���s��?���a����./7��e���r�����R���?N�.�N�q��Fc<�?�\\�������������b>y������xr`z��-��v�^���4;.��:�&�Z��8A;�W��pH+v�w3U�&&C?�9?}{q���Nj��0e�����/P��J��*�����#E�a�6=�V�i	��N����Bt��������[��U�[E��T��J������i�F�����.�k��e���1�1`e^Y_/C��4W1��M������|��|P��1����7oTE�B%���}M�����z����z��W����o�N��O��)���J��"�&x�c@7b 
�U�����:y���r$��X\����7��h�V�vzY@��O�j���f��~���0mq����,�dc
T�E�I�4�W6�A��d�L�Q��A�#���DH�y�����r�x�W��v�N$�.����^����{�F�|y�z�����f2��r����Y>�z�]�M���,��d>�m��4"�F�s���,�����Q�$��(98P�Kxn��wz�
�����t}�w����ga�{		;�k����-$O�DG��`I�����{=����rn����g�q�s)x
q����!�?����0y�������i��q7G�1��m
Q��h��C����j2{:����U���mm�t�?&�/?�y�/bfN���d���fk�����(���K�i��������
�n���|�y����I
����e'o�s�����U��W�U�R���N3L7%�L�d`)C�lx����i�a��3���U��W�U�,�|e'|���)?3�E3��T6�5�r�H�In��g#���n��|�������vE^C����_�u�G�����������Al��<{s��Wg?��2gc����K�!���!E���r�H8�hWH���RTL�R�)��{�0h�>f/�r��`����(��v�Q�B�M������\�B�A:6i7Gu�f,���X^M�0�a���k���J=��@�q
����}��o��qL����t�G���+5:�'0�dWx.���Ws�Eji�I.B��Y=���������4�����
A��Oi���1jg-�o�%����[�����%wc.B��Y���k7r�U~�Eo������f����6W��F5g��s�D���r���.����5����D���v����4����u|���1��_v�u�LVSr��6�a{�;����5LCdt�����s>#�k����o�*�!�m�:0��q���f]R���p���a�aF0�:���t�X������9��4Qo�,3�e&���t�i�Xmc��a��1�&tX]p
E1�[��	�d�'3]����P��{W�l(�y��f~'"� �!��$=�s��
{]p
�*k�T]�+����1Y��Gk��db[w��)�����.�6��s����a��MZ�M6iLY[�v�MO��gCk�-M&�6��w@�n�z��5s�>m��`��t
0k������*G�4�4���������0=��siCk�[�Z�m�������i������M�k������������9kCk�������M#l$������h���;�6���m1C������y�\�K���������%`�M����M.G�����z���q���X\n���R�\�J�f��w��������W�#�O�����g��>9���
�u,��S�����/�H�-�e�6�a�[��|�U���;|�#c��t�z�s[�e- ��h����]�����C^��%��j�.���*]�4���@�:��M����tg��GK"j:u)iNJ�����lw'n>�J5[����������~���d���kw�BpW&w��[rTS����|m��z��5�r�K��c:��#�6_)�f�)����>[����S�����|=(��\�p���4�-�.%��i�)�����<w��������t�����a�d�KE"��PB������5W������R�%�~�F�m��\2�{8�nW�{�w�B�f���Dm�7��b���V�����L��Y+���>��W}R�l���npD��!+n�U��T�:
�`L���a������:La�)C0��T�T��6�[};����`]�q�!.�B��r��BaG�w�u���o�8N����G�.�.;;�
���6�Z~m[4k���� 
�b�l-�1�����Alg��h�A��N1u��u�
Zt�����o~o��jz}�L�Z^�����tP#�����
8����$Z��	���t����^��e�|3�_-'�5?������u��7����������Z�������������������t^$������@�#@�N |N�s�U��2!$H��?�={�)��b��������zq����<G�X�O�����t��'O��VyV�E��O�r��4��].'�"X�*UW����?u�q����[����������_�Fsoe���zt?[�`��CN.����?��O|���FOU��#�]��+��u������W���7j���o��T�;�9ys~r�s��i�[B�j�[u����� �����������v���A�YY��,��
�3\���v�^h3[�+Y�@o=KC���p|�>zF��|�~�:��W�:Z��
��P�W����!��D&0Ap��J0��:AUh&H��H)�	�YYG}`��k_kz��U6r!�
;+e�N���Z6 qdC��`6h��z6`��3�gX#�l`���B���=a���
Z�:�!�
"��;�o�E�
<�&���cC?�0��86"��@	�l ����o)�-E �k)��GK�8�#L��#� 6�n�a*�h/F�X��=��"�
z��eCO�DB;�	�q&2�
Db3��������'L%��OAl`��>)�`?f���O3l`@e)�!�
�|���!�6 ��
��oc��8X6@��~�H���X7���@W7 ���Af��`!d\��	�Y����X����[6H�"����e���'l ��0%Jw8�����[(N�_�M/������O.��L��2��P��0i+"b��~N%EHo<Re���2z����Z��z������La'����S�@�>�J��G�06@j?�JU���
z6���6�
 ������"����!&�
�5�
:�������
qB%�
X6�dB�0l��f��t����h�����^?&�������x��������+�j�FI���r����w��y��|�`'�h��(2�I�G��$ug����H�`0�RV�.�R�� �L ��O�
�q �w�O�A���P
8$�i���s ���J�@z[�����k�'���di�v�pI�z�������"�p�{Q������w�_�D:?V�><<���O�v����j�`����BfhmQ���#�����If��u��2��D&�A+�]��s<K>tM���A�]c�,x�,����h6�,�������[�f��@+�������T�V��Go\SM:���[&�kC��r&���������d~9��(�Gn�s{kP���������y�'��}���u����i4.����c���A\U�FGUW�Y�8K���R!�0�#���m��s��Uq��6v�8�QU�su��'�}��������?���->�N��6U�1���Q�tgmS�%j�ri*��m����������m���M�<C<j�~zmS�����Mw�6U �D�Mw�6EZ�&
"����'��Ng**�<M�T?���}�b��R�����Q�������E��<��XPL�����I�����)?��QE+�������B��EF���S�SBkVYo������?0K}T���@��^�?9dV�S�6��"�����9�= ����<.�S���(��vA��� ��M��(�$������T�iOD@P��a�S��8�j�u��vod�T:2`�02`a����~���A U�#26�
^^���>42�vAJ�.�020LX����G������ ��#�}�[�������B8����*C�7L�f"��f��^4H2'����P�G)�v�:�0p3N*Gq4F���bE?F�"�F�� 2P�E�T�q_d`�M:aQ�t
U�N��b���Y4/���E���~}��Vr�
-P�3���Zw�
:��>c�����!������^�y?TR��F2�;:?Tv�6zodH]�H,"���]�H���20����P\X7�>?!���RIX/F���..)���ad����K
;U��V���~GT���_���2��I�.���������,rO���;����}�?���!����S�J��������Gl�;���^]�8.�O��C}g;����2i������?6I���jl t���[��}�R���@D��}�h,�_�t�3�*C��@!�H'51�E�C�qR����BW�ye��PQ���1
���z!
���%������������%��
KdT��tg�����;B+ � Q@8
G�a$8v�HKD�( ���0�v b`��j�J@iOBSX'��;F���:%�( ���0R�
�85�G���H�D�bna"%V@1��/3:K�;�`�D�g�$��x|��6t��L����<����g�����8��i��3{�1��2��vg�g���3N�����6����3�_V�������W�����f��f����N����zkz��?z�����@2���g2�@�+0�D���Y����
8��d��j,��J�FO@����#�K�{�,0=�fFe�R$�������N����;�g&�s�@2Hf�����=�����Ny��N��3��}h&T�W�C!�9B�!��
����{�_����d���?4+;�.��r�Y1�t�U=��?����n�o����	7,u.�b�KK}8p��nuM�V�x�V��F�����`���:�������k�Adp�3�Du��uod`�)�]��!dP�hK��>4�JD��(KF������$�d�QN(�Z���N����P2�2CD)��O2R8��[)�}�������,�^���$��f�w5�$�ad@���~��?Ok�02`��^A'{Q3�D�0A�J��K��n�������Z�)x����0*v=�=������`�r��S��2V%!�w�������������k�x��#�����������'�,�8�� ��qR�{��d����B��!��
t>6������ @�3��Ahg5�@��>�@'��Q��$_�Qz���>�"�� c3����%�h&T������w�E�;��G
��^|.W"��(~.%�����	$�)�CH���02�hVUH��$�*�8j�Fm��@�� �����CnX���VS���C.APx2zE$�E���+������P$C����H�2p���c3HID?�	�u������3��";�V
N/������*G8N���~�'���~�d��9�����0����C���`S�<D������� ��E�
��M(�,N����/z
�)"�i+��V�\-�w}����{���r<IN����7���>���kU4^�;u���m	�=�}�Z���������FU����L����->��C��}�l~��,��*��a%�����i����o��L1Et��7��}���N�m�x����(
q�!^�S)�F��E��RI���e�o"6C`
b/4�t���<��F�7J<��x�#��G<��x�#��G<��x���<�FM�b�
#9Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Stephen Frost (#6)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

Stephen Frost wrote:

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.

Yeah. Trying to compile old stuff with current tools (Debian jessie):

7.0's configure does not recognize my system:

checking host system type... Invalid configuration `x86_64-unknown-linux-gnu': machine `x86_64-unknown' not recognized

7.1's configure fails for accept() detection:

checking types of arguments for accept()... configure: error: could not determine argument types

7.2's configure works, but apparently it failed to find flex:

make[3]: Entering directory '/home/alvherre/Code/pgsql/source/throwaway/src/backend/bootstrap'
***
ERROR: `flex' is missing on your system. It is needed to create the
file `bootscanner.c'. You can either get flex from a GNU mirror site
or download an official distribution of PostgreSQL, which contains
pre-packaged flex output.
***
Makefile:60: recipe for target 'bootscanner.c' failed
make[3]: *** [bootscanner.c] Error 1

7.3 fails in ecpg preprocessor:

make[4]: Entering directory '/home/alvherre/Code/pgsql/source/throwaway/src/interfaces/ecpg/preproc'
make -C ../../../../src/port all
make[5]: Entering directory '/home/alvherre/Code/pgsql/source/throwaway/src/port'
make[5]: Nothing to be done for 'all'.
make[5]: Leaving directory '/home/alvherre/Code/pgsql/source/throwaway/src/port'
gcc -O2 -fno-strict-aliasing -Wall -Wmissing-prototypes -Wmissing-declarations -Wno-error -I./../include -I. -I../../../../src/include -DMAJOR_VERSION=2 -DMINOR_VERSION=10 -DPATCHLEVEL=0 -DINCLUDE_PATH=\"/usr/local/pgsql/include\" -c -o preproc.o preproc.c
In file included from preproc.y:5571:0:
pgc.c:170:18: error: conflicting types for ‘yyleng’
extern yy_size_t yyleng;
^
In file included from preproc.y:7:0:
extern.h:32:4: note: previous declaration of ‘yyleng’ was here
yyleng;
^
In file included from preproc.y:5571:0:
pgc.c:304:11: error: conflicting types for ‘yyleng’
yy_size_t yyleng;
^
In file included from preproc.y:7:0:
extern.h:32:4: note: previous declaration of ‘yyleng’ was here
yyleng;
^
In file included from preproc.y:5571:0:
pgc.c:2723:16: warning: ‘input’ defined but not used [-Wunused-function]
static int input (void)
^
<builtin>: recipe for target 'preproc.o' failed
make[4]: *** [preproc.o] Error 1

7.4 seems to work fine. I suppose it should be fine to remove pg_dump's
support for pre-7.3; people wanting to upgrade from before 7.3 (if any)
could use 9.6's pg_dump as an intermediate jump.

--
Á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

#10Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#1)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

Hi,

On 2016-05-03 12:07:51 -0400, Tom Lane wrote:

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.

Alternatively we could just add a function for adding a toast table -
that seems less hacky and less likely to be broken in the future.

- Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11Tom Lane
tgl@sss.pgh.pa.us
In reply to: Stephen Frost (#6)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

Stephen Frost <sfrost@snowman.net> writes:

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...

Yeah. I do periodically fire up servers back to 7.0 and see if pg_dump
can dump from them, but I don't pretend that that's a very thorough test.

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.

I've been thinking of proposing that it's time (not now, at this point,
but for 9.7) to rip out libpq's support for V2 protocol as well as
pg_dump's support for pre-7.4 backends. That's a quite significant
amount of what at this point is very poorly tested code. And I doubt
that it would be productive to try to improve the test coverage rather
than just removing it.

There might be an argument for moving pg_dump's cutoff further than that,
but going to 7.3 or later is significant because it would allow removal of
the kluges for schema-less and dependency-less servers. I suggested 7.4
because it'd comport with removal of V2 wire protocol support, and because
7.4 is also our cutoff for describe support in psql.

I'm hesitant to move the cutoff really far, because we do still hear from
people running really old versions, and sooner or later those people will
want to upgrade. It'd be good if they could use a modern pg_dump for the
purpose.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12Stephen Frost
sfrost@snowman.net
In reply to: Andres Freund (#10)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

* Andres Freund (andres@anarazel.de) wrote:

On 2016-05-03 12:07:51 -0400, Tom Lane wrote:

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.

Alternatively we could just add a function for adding a toast table -
that seems less hacky and less likely to be broken in the future.

+1

Thanks!

Stephen

#13Andres Freund
andres@anarazel.de
In reply to: Tom Lane (#11)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

On 2016-05-03 13:47:14 -0400, Tom Lane wrote:

I've been thinking of proposing that it's time (not now, at this point,
but for 9.7) to rip out libpq's support for V2 protocol as well as
pg_dump's support for pre-7.4 backends.

+1

There might be an argument for moving pg_dump's cutoff further than that,
but going to 7.3 or later is significant because it would allow removal of
the kluges for schema-less and dependency-less servers. I suggested 7.4
because it'd comport with removal of V2 wire protocol support, and because
7.4 is also our cutoff for describe support in psql.

I think we can be a lot more aggressive moving the cuttoff for psql than
for pg_dump; but that's more an argument ripping out some old psql code.

I'm hesitant to move the cutoff really far, because we do still hear from
people running really old versions, and sooner or later those people will
want to upgrade. It'd be good if they could use a modern pg_dump for the
purpose.

I think we should consider making the cutoff point for pg_dump somewhat
predicatable. E.g. saying that we support 5 more versions than the
actually maintained ones. The likelihood of breakages seems to
increase a good bit for older versions.

Andres

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#14Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Andrew Dunstan (#8)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

Andrew Dunstan wrote:

And if this is of any use, here are the dump differences from every live
version to git tip, as of this morning.

Interesting, thanks. I wonder if some of these diffs could be reduced
further by using pg_dump -Fd instead of a single text dump -- then
internal ordering would not matter, and I see that a large part of these
diffs is where GRANTs appear. (I don't think it's a problem to use a
newer pg_dump to dump the older databases that don't support -Fd, for
this purpose.)

How would you recommend to run this in the coverage reporting machine?
Currently it's just doing "make check-world". Could we add a buildfarm
script to run, standalone?

--
�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

#15Tom Lane
tgl@sss.pgh.pa.us
In reply to: Andres Freund (#10)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

Andres Freund <andres@anarazel.de> writes:

On 2016-05-03 12:07:51 -0400, Tom Lane wrote:

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.

Alternatively we could just add a function for adding a toast table -
that seems less hacky and less likely to be broken in the future.

We used to have an explicit "ALTER TABLE CREATE TOAST TABLE", IIRC,
but we got rid of it. In any case, forcible creation is not what
we're after here, we just want to create it if needs_toast_table()
thinks we need one. I'm fine with it being a hack, as long as we
have test coverage so we notice when somebody breaks the hack.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#16Andrew Dunstan
andrew@dunslane.net
In reply to: Alvaro Herrera (#14)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

On 05/03/2016 01:58 PM, Alvaro Herrera wrote:

Andrew Dunstan wrote:

And if this is of any use, here are the dump differences from every live
version to git tip, as of this morning.

Interesting, thanks. I wonder if some of these diffs could be reduced
further by using pg_dump -Fd instead of a single text dump -- then
internal ordering would not matter, and I see that a large part of these
diffs is where GRANTs appear. (I don't think it's a problem to use a
newer pg_dump to dump the older databases that don't support -Fd, for
this purpose.)

How would you recommend to run this in the coverage reporting machine?
Currently it's just doing "make check-world". Could we add a buildfarm
script to run, standalone?

Well, to run it you just run a buildfarm animal, possibly not even
registered, with the module enabled. The module is actually in every
buildfarm release, and has been for some time, but it's not enabled.
Right now even if it runs it doesn't report anything to the server, it
just outputs success/failure lines to stdout.

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

#17Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#1)
1 attachment(s)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

I wrote:

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

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.

I tried fixing it like that. The alternate RESET target had behaved as
expected when I'd tested by hand, but in pg_upgrade it still fails,
only now with

Creating newly-required TOAST tables SQL command failed
ALTER TABLE "public"."i_need_a_toast_table" RESET (user_catalog_table);
ERROR: pg_type OID value not set when in binary upgrade mode

This implies that there was some totally other patch, probably quite
pg_upgrade-specific, that broke this case independently of the
lock-downgrade change.

My conclusion is that we probably do need a specific pg_upgrade support
function to handle the case, rather than trying to sneak it through via
ALTER TABLE, which means that we won't be able to back-patch a fix.

I have no more time to work on this, but I think it needs to be fixed, and
I definitely think we had better put in test coverage when we do fix it.
Attached is a proposed patch that adds regression test coverage for this
and a related case (and triggers the failures I've been complaining of).

regards, tom lane

Attachments:

pg_upgrade-toast-tests.patchtext/x-diff; charset=us-ascii; name=pg_upgrade-toast-tests.patchDownload
diff --git a/src/test/regress/expected/indirect_toast.out b/src/test/regress/expected/indirect_toast.out
index 4f4bf41..3ed0189 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,177 ----
  
  DROP TABLE toasttest;
  DROP FUNCTION update_using_indirect();
+ --
+ -- Create a couple of tables that have unusual TOAST situations, and leave
+ -- them around so that they'll be in the final regression database state.
+ -- This enables testing of these scenarios for pg_upgrade.
+ --
+ -- Table that has a TOAST table, but doesn't really need it.
+ create table i_have_useless_toast_table(f1 int, f2 text);
+ insert into i_have_useless_toast_table values(1, 'foo');
+ alter table i_have_useless_toast_table drop column f2;
+ -- Table that needs a TOAST table and has not got one.  This is uglier...
+ -- we can't actually remove the TOAST table, only unlink it from parent.
+ -- But leaving an orphan TOAST table is good for testing pg_upgrade, anyway.
+ create table i_need_a_toast_table(f1 int, f2 text);
+ insert into i_need_a_toast_table values(1, 'foo');
+ update pg_class set reltoastrelid = 0
+   where relname = 'i_need_a_toast_table';
+ SELECT relname, reltoastrelid <> 0 AS has_toast_table
+    FROM pg_class
+    WHERE oid::regclass IN ('i_have_useless_toast_table', 'i_need_a_toast_table')
+    ORDER BY 1;
+           relname           | has_toast_table 
+ ----------------------------+-----------------
+  i_have_useless_toast_table | t
+  i_need_a_toast_table       | f
+ (2 rows)
+ 
diff --git a/src/test/regress/sql/indirect_toast.sql b/src/test/regress/sql/indirect_toast.sql
index d502480..15640fc 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,85 ----
  
  DROP TABLE toasttest;
  DROP FUNCTION update_using_indirect();
+ 
+ --
+ -- Create a couple of tables that have unusual TOAST situations, and leave
+ -- them around so that they'll be in the final regression database state.
+ -- This enables testing of these scenarios for pg_upgrade.
+ --
+ 
+ -- Table that has a TOAST table, but doesn't really need it.
+ create table i_have_useless_toast_table(f1 int, f2 text);
+ insert into i_have_useless_toast_table values(1, 'foo');
+ alter table i_have_useless_toast_table drop column f2;
+ 
+ -- Table that needs a TOAST table and has not got one.  This is uglier...
+ -- we can't actually remove the TOAST table, only unlink it from parent.
+ -- But leaving an orphan TOAST table is good for testing pg_upgrade, anyway.
+ create table i_need_a_toast_table(f1 int, f2 text);
+ insert into i_need_a_toast_table values(1, 'foo');
+ update pg_class set reltoastrelid = 0
+   where relname = 'i_need_a_toast_table';
+ 
+ SELECT relname, reltoastrelid <> 0 AS has_toast_table
+    FROM pg_class
+    WHERE oid::regclass IN ('i_have_useless_toast_table', 'i_need_a_toast_table')
+    ORDER BY 1;
#18Tom Lane
tgl@sss.pgh.pa.us
In reply to: Tom Lane (#17)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

I wrote:

I have no more time to work on this, but I think it needs to be fixed, and
I definitely think we had better put in test coverage when we do fix it.

Actually, there is a really easy fix we could put in, which is to decide
that optionally_create_toast_tables() is useless and get rid of it.

Point 1: if a table did not have a TOAST table in the old database,
any decision that it needs one in the new database must be a very
close-to-the-edge situation; certainly the 9.2 change we had in this area
was a matter of rounding things off differently. needs_toast_table()'s
threshold for max tuple length is only a quarter page, so there's a huge
amount of daylight between where we'd choose to create a toast table and
where users would actually see failures from not having one. It's pretty
hard to believe that cross-version differences in the do-we-need-a-toast-
table heuristic would exceed that.

Point 2: the current code is broken, and will cause a pg_upgrade failure
if the situation of concern occurs. That's certainly much worse than
not adding a marginally-useful toast table.

Point 3: in view of point 2, the lack of field complaints says that this
situation doesn't actually happen in the field.

Barring complaints, I'll fix this bug by removing that code altogether.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#19Simon Riggs
simon@2ndQuadrant.com
In reply to: Tom Lane (#1)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

On 3 May 2016 at 18:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Or at least, it did until Simon decided that ALTER TABLE RESET
doesn't require AccessExclusiveLock.

On reflection, this still seems like a good idea.

Now you get a failure.

Failure condition as an exception to that.

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

It appears that pg_upgrade is depending upon an undocumented side-effect of
ALTER TABLE RESET.

I would say this side-effect should not exist, which IIUC is the same
conclusion on your latest post.

If pg_upgrade needs this, we should implement a specific function that does
what pg_upgrade needs. That way we can isolate the requirement for an
AccessExclusiveLock to the place that needs it: pg_upgrade. That will also
make it less fragile in the future. I don't think that needs a specific
command, just a function.

I accept that it is my bug and should fix it.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#20Tom Lane
tgl@sss.pgh.pa.us
In reply to: Simon Riggs (#19)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

Simon Riggs <simon@2ndquadrant.com> writes:

On 3 May 2016 at 18:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Or at least, it did until Simon decided that ALTER TABLE RESET
doesn't require AccessExclusiveLock.

On reflection, this still seems like a good idea.

Yes, what pg_upgrade was doing was clearly a hack, and not a very nice one.

I accept that it is my bug and should fix it.

It's moot at this point, see 1a2c17f8e.

regards, tom lane

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#21Simon Riggs
simon@2ndQuadrant.com
In reply to: Tom Lane (#20)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

On 7 May 2016 at 16:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Simon Riggs <simon@2ndquadrant.com> writes:

On 3 May 2016 at 18:07, Tom Lane <tgl@sss.pgh.pa.us> wrote:

Or at least, it did until Simon decided that ALTER TABLE RESET
doesn't require AccessExclusiveLock.

On reflection, this still seems like a good idea.

Yes, what pg_upgrade was doing was clearly a hack, and not a very nice one.

I accept that it is my bug and should fix it.

It's moot at this point, see 1a2c17f8e.

OK, sorry for the noise and thanks for the fix.

--
Simon Riggs http://www.2ndQuadrant.com/
<http://www.2ndquadrant.com/&gt;
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

#22Greg Stark
stark@mit.edu
In reply to: Alvaro Herrera (#9)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

For what it's worth, for my historical sort benchmarks I got Postgres
to build right back to 6.5 using modern tools. I have patches if
anyone wants them. Pre-7.3 doesn't actually run because we didn't
support 64-bit architectures before Tom did the V1 api (there was a
set of Alpha patches floating around but they don't seem sufficient
for x86_64). But I suspect if I built it for x86 32-bit that would
clear the immediate problem.

I was considering proposing backpatching some minimal patches to get
old unsupported branches to at least build with modern tools and run.
Just to make it easy for people to test historical behaviour and I
suppose pg_dump or other client testing would be a good use case too.
I was also going to propose turning off all warnings on these
unsupported back branches while I was at it.

One other lesson I learned doing this was that it was a pain referring
to individual git checkouts because we don't have a tag for point
where we branched the new development. So all my git-describes were
ending up with things like REL9_2_BETA2-619-gff6c78c or
REL9_3_BETA1-925-g6668ad1. You just had to know that REL9_3_BETA1-xxx
was probably a revision sometime during PG 9.5 development since BETA1
was probably where we forked development whereas 9.3 probably forked
off 9_2_BETA2. (And some were things like REL7_4_RC1-1513-g4525418
instead)

I would suggest adding tags for each version on the first development
revision so that we could see things like REL9_5_DEV-nnn would mean
the nnnth commit on the 9.5 development branch.

(What I actually did instead myself was use git describe --tags
--contains $i --match 'REL[0-9]_[0-9]_[0-9]' which gave me things like
REL9_5_0~1334 which means the 1334th commit *before* REL9_5_0. That
was also confusing but at least consistent)

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#23Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#1)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

On Tue, May 3, 2016 at 12:07:51PM -0400, Tom Lane wrote:

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?

FYI, I only test _supported_ version combinations for pg_upgrade, i.e. I
don't test pg_upgrade _from_ unsupported versions, though I can see why
maybe I should.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+                     Ancient Roman grave inscription +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#24Bruce Momjian
bruce@momjian.us
In reply to: Tom Lane (#17)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

On Fri, May 6, 2016 at 03:32:23PM -0400, Tom Lane wrote:

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.

I tried fixing it like that. The alternate RESET target had behaved as
expected when I'd tested by hand, but in pg_upgrade it still fails,
only now with

Creating newly-required TOAST tables SQL command failed
ALTER TABLE "public"."i_need_a_toast_table" RESET (user_catalog_table);
ERROR: pg_type OID value not set when in binary upgrade mode

I think this means that the ALTER TABLE RESET is adding or potentially
adding a pg_type row, and no one called
binary_upgrade_set_next_pg_type_oid().

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+                     Ancient Roman grave inscription +

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#25Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Alvaro Herrera (#4)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

On 5/3/16 1:25 PM, Alvaro Herrera wrote:

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.

Not that that would be useless, but note that the value in this case
(and most others) comes from having a candidate object in the database
before upgrade that exercises the particular problem, mostly independent
of what version you upgrade from and to. So far the way to do that is
to leave "junk" in the regression test database, but that's clearly a
bit silly.

I think the way forward is to create a TAP test suite for pg_upgrade
that specifically exercises a lot of scenarios with small purpose-built
test databases.

Then, the problem of having to compare dump output across versions also
goes away more easily.

--
Peter Eisentraut 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

#26Bruce Momjian
bruce@momjian.us
In reply to: Peter Eisentraut (#25)
1 attachment(s)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

On Wed, May 11, 2016 at 09:40:09AM -0400, Peter Eisentraut wrote:

Not that that would be useless, but note that the value in this case (and
most others) comes from having a candidate object in the database before
upgrade that exercises the particular problem, mostly independent of what
version you upgrade from and to. So far the way to do that is to leave
"junk" in the regression test database, but that's clearly a bit silly.

I think the way forward is to create a TAP test suite for pg_upgrade that
specifically exercises a lot of scenarios with small purpose-built test
databases.

Then, the problem of having to compare dump output across versions also goes
away more easily.

I do have some small tests like for tablespaces. I am attaching the SQL
script, if that is helpful.

--
Bruce Momjian <bruce@momjian.us> http://momjian.us
EnterpriseDB http://enterprisedb.com

+ As you are, so once was I. As I am, so you will be. +
+                     Ancient Roman grave inscription +

Attachments:

pg_upgrade_test.sqlapplication/x-sqlDownload
#27Alvaro Herrera
alvherre@2ndquadrant.com
In reply to: Peter Eisentraut (#25)
Re: ALTER TABLE lock downgrades have broken pg_upgrade

Peter Eisentraut wrote:

On 5/3/16 1:25 PM, Alvaro Herrera wrote:

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.

Not that that would be useless, but note that the value in this case (and
most others) comes from having a candidate object in the database before
upgrade that exercises the particular problem, mostly independent of what
version you upgrade from and to. So far the way to do that is to leave
"junk" in the regression test database, but that's clearly a bit silly.

True. We have quite a few places in the standard regression tests that
leave junk behind purposefully for this reason.

I think the way forward is to create a TAP test suite for pg_upgrade that
specifically exercises a lot of scenarios with small purpose-built test
databases.

That works for me.

Then, the problem of having to compare dump output across versions also goes
away more easily.

Not sure why, but if you think it does, then it sounds good. Andrew's
current approach of counting lines in the diff seems brittle and not
entirely trustworthy.

--
�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