Re: pgsql: Add pg_alterckey utility to change the cluster key
On Sat, Dec 26, 2020 at 02:00:02PM -0500, Bruce Momjian wrote: > On Sat, Dec 26, 2020 at 12:18:18PM -0500, Bruce Momjian wrote: >> I can easily revert and come back, though the buildfarm is green now. >> As far as testing, I can test that the cluster key unlocks the data >> keys, but there is no current interface to the data keys. Ideally we >> would test the full input/output path, but with no access to the output, >> how can we test it? Also, as I stated, there are some shell script APIs >> that can't easily be tested, e.g. AWS, Yubikey. I can continue to test >> those manually. It seems to me that it is better to figure out that while the feature is being developed, not after committing it so as there are fully-functional tests at the same time the feature is committed. If we don't have that in place, how can people know the amount of testing that has been done for this feature? And how can anybody be sure that nothing breaks if this area of the code gets changed? Manual testing does not scale. For example, I have seen cases in the past where implementing tests improved the whole state of a feature design because it was possible to finish with a more flexible set of methods for this feature. Based on the number of concerns raised by various people over the last couple of days (including myself, one point being the refactoring of the ciphers taken from pgcrypto that should have been in its own commit), I agree that it would be better to revert this code for now. -- Michael signature.asc Description: PGP signature
Re: pgsql: Add pg_alterckey utility to change the cluster key
On Sun, Dec 27, 2020 at 05:48:47PM +0900, Michael Paquier wrote: > On Sat, Dec 26, 2020 at 02:00:02PM -0500, Bruce Momjian wrote: > > On Sat, Dec 26, 2020 at 12:18:18PM -0500, Bruce Momjian wrote: > >> I can easily revert and come back, though the buildfarm is green now. > >> As far as testing, I can test that the cluster key unlocks the data > >> keys, but there is no current interface to the data keys. Ideally we > >> would test the full input/output path, but with no access to the output, > >> how can we test it? Also, as I stated, there are some shell script APIs > >> that can't easily be tested, e.g. AWS, Yubikey. I can continue to test > >> those manually. > > It seems to me that it is better to figure out that while the feature > is being developed, not after committing it so as there are > fully-functional tests at the same time the feature is committed. If > we don't have that in place, how can people know the amount of testing > that has been done for this feature? And how can anybody be sure that > nothing breaks if this area of the code gets changed? Manual testing > does not scale. For example, I have seen cases in the past where > implementing tests improved the whole state of a feature design > because it was possible to finish with a more flexible set of methods > for this feature. > > Based on the number of concerns raised by various people over the last > couple of days (including myself, one point being the refactoring of > the ciphers taken from pgcrypto that should have been in its own > commit), I agree that it would be better to revert this code for now. OK, I will do so in the next few hours. My followup will be to: * register it for the commit-fest so it gets cfbot and other visibility * modify pgcrypto to use the new AES API (the SHA512 call no longer exists) * develop TAP tests, though as I mentioned, they will be odd -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
pgsql: Stabilize test introduced in 05c02589, per buildfarm.
Stabilize test introduced in 05c02589, per buildfarm. In passing, make the capitalization match the rest of the file. Reported-by: Tom Lane Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/fa0fdf0510df1a21f42ac9f232f77a79b8577152 Modified Files -- src/test/regress/expected/groupingsets.out | 16 +--- src/test/regress/sql/groupingsets.sql | 16 +--- 2 files changed, 18 insertions(+), 14 deletions(-)
Re: pgsql: Fix bug #16784 in Disk-based Hash Aggregation.
On Sun, 2020-12-27 at 02:09 -0500, Tom Lane wrote: > Jeff Davis writes: > > Fix bug #16784 in Disk-based Hash Aggregation. > > Buildfarm thinks this new test case is not completely stable. > > It appears that only 32-bit machines are failing, and it's > not totally consistent even for them. Dunno what to make of it. I think I just need to disable sort. Because there is a () group, we don't need a Sort in the plan to exercise the bug. Committed to master. I'll backport if the tests stabilize. I'm on vacation now and will return on the 5th, so hopefully we're in a reasonable state until then. If not, I can just revert my fix and deal with it when I get back. Regards, Jeff Davis
pgsql: Second attempt to stabilize 05c02589.
Second attempt to stabilize 05c02589. Removing the EXPLAIN test to stabilize the buildfarm. The execution test should still be effective to catch the bug even if the plan is slightly different on different platforms. Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/facad31474ac6dace3894ebc7c45dc3cc829422e Modified Files -- src/test/regress/expected/groupingsets.out | 19 --- src/test/regress/sql/groupingsets.sql | 5 - 2 files changed, 24 deletions(-)
pgsql: Second attempt to stabilize 05c02589.
Second attempt to stabilize 05c02589. Removing the EXPLAIN test to stabilize the buildfarm. The execution test should still be effective to catch the bug even if the plan is slightly different on different platforms. Branch -- REL_13_STABLE Details --- https://git.postgresql.org/pg/commitdiff/cd7d8cde75063a85ee8c5fd27713061e56a8684d Modified Files -- src/test/regress/expected/groupingsets.out | 19 --- src/test/regress/sql/groupingsets.sql | 5 - 2 files changed, 24 deletions(-)
pgsql: Stabilize test introduced in 05c02589, per buildfarm.
Stabilize test introduced in 05c02589, per buildfarm. In passing, make the capitalization match the rest of the file. Reported-by: Tom Lane Branch -- REL_13_STABLE Details --- https://git.postgresql.org/pg/commitdiff/6669cc769c44b691510c14be12acd9148c74d4d1 Modified Files -- src/test/regress/expected/groupingsets.out | 16 +--- src/test/regress/sql/groupingsets.sql | 16 +--- 2 files changed, 18 insertions(+), 14 deletions(-)
Re: pgsql: Fix bug #16784 in Disk-based Hash Aggregation.
On Sun, 2020-12-27 at 09:59 -0800, Jeff Davis wrote: > I think I just need to disable sort. Because there is a () group, we > don't need a Sort in the plan to exercise the bug. That still didn't stabilize the test, so I removed the EXPLAIN part of the test and left the execution test in place. The planning for grouping sets still uses work_mem in the knapsack algorithm, so something about the other platforms is not considering the plan with all hashes (except the () grouping set). I think we can tune the test to be stable, but it doesn't seem critical so I think we're fine just doing the execution-time test. Regards, Jeff Davis
pgsql: Revert "Add key management system" (978f869b99) & later commits
Revert "Add key management system" (978f869b99) & later commits The patch needs test cases, reorganization, and cfbot testing. Technically reverts commits 5c31afc49d..e35b2bad1a (exclusive/inclusive) and 08db7c63f3..ccbe34139b. Reported-by: Tom Lane, Michael Paquier Discussion: https://postgr.es/m/[email protected] Branch -- master Details --- https://git.postgresql.org/pg/commitdiff/3187ef7c46c5b884267a88f2d6119c9a05f1bbba Modified Files -- doc/src/sgml/config.sgml | 96 +--- doc/src/sgml/database-encryption.sgml | 97 doc/src/sgml/filelist.sgml| 1 - doc/src/sgml/installation.sgml| 5 +- doc/src/sgml/postgres.sgml| 1 - doc/src/sgml/ref/allfiles.sgml| 3 +- doc/src/sgml/ref/initdb.sgml | 46 -- doc/src/sgml/ref/pg_alterckey.sgml| 197 doc/src/sgml/ref/pg_ctl-ref.sgml | 14 - doc/src/sgml/ref/pgupgrade.sgml | 20 +- doc/src/sgml/ref/postgres-ref.sgml| 13 - doc/src/sgml/reference.sgml | 1 - doc/src/sgml/storage.sgml | 5 - src/backend/Makefile | 17 +- src/backend/access/transam/xlog.c | 21 - src/backend/bootstrap/bootstrap.c | 21 +- src/backend/crypto/Makefile | 18 - src/backend/crypto/ckey_aws.sh.sample | 50 -- src/backend/crypto/ckey_direct.sh.sample | 37 -- src/backend/crypto/ckey_passphrase.sh.sample | 33 -- src/backend/crypto/ckey_piv_nopin.sh.sample | 63 --- src/backend/crypto/ckey_piv_pin.sh.sample | 76 --- src/backend/crypto/kmgr.c | 372 -- src/backend/crypto/ssl_passphrase.sh.sample | 33 -- src/backend/libpq/be-secure-common.c | 14 - src/backend/main/main.c | 3 - src/backend/postmaster/pgstat.c | 9 - src/backend/postmaster/postmaster.c | 13 +- src/backend/replication/basebackup.c | 5 - src/backend/storage/ipc/ipci.c| 3 - src/backend/storage/lmgr/lwlocknames.txt | 1 - src/backend/tcop/postgres.c | 25 +- src/backend/utils/misc/guc.c | 24 - src/backend/utils/misc/pg_controldata.c | 11 +- src/backend/utils/misc/postgresql.conf.sample | 5 - src/bin/Makefile | 1 - src/bin/initdb/initdb.c | 118 + src/bin/pg_alterckey/.gitignore | 1 - src/bin/pg_alterckey/Makefile | 38 -- src/bin/pg_alterckey/pg_alterckey.c | 694 -- src/bin/pg_controldata/pg_controldata.c | 3 - src/bin/pg_ctl/pg_ctl.c | 59 +-- src/bin/pg_resetwal/pg_resetwal.c | 2 - src/bin/pg_rewind/filemap.c | 8 - src/bin/pg_upgrade/check.c| 34 -- src/bin/pg_upgrade/controldata.c | 42 +- src/bin/pg_upgrade/file.c | 2 - src/bin/pg_upgrade/option.c | 7 +- src/bin/pg_upgrade/pg_upgrade.h | 3 - src/bin/pg_upgrade/server.c | 5 +- src/common/Makefile | 3 - src/common/cipher.c | 70 --- src/common/cipher_openssl.c | 268 -- src/common/kmgr_utils.c | 507 --- src/include/catalog/pg_control.h | 5 +- src/include/common/cipher.h | 62 --- src/include/common/kmgr_utils.h | 98 src/include/crypto/kmgr.h | 29 -- src/include/pgstat.h | 3 - src/include/postmaster/postmaster.h | 2 - src/include/utils/guc_tables.h| 1 - src/tools/msvc/Mkvcbuild.pm | 4 +- 62 files changed, 52 insertions(+), 3370 deletions(-)
Re: pgsql: Add pg_alterckey utility to change the cluster key
On Sun, Dec 27, 2020 at 12:44:50PM -0500, Bruce Momjian wrote: > > Based on the number of concerns raised by various people over the last > > couple of days (including myself, one point being the refactoring of > > the ciphers taken from pgcrypto that should have been in its own > > commit), I agree that it would be better to revert this code for now. > > OK, I will do so in the next few hours. My followup will be to: > > * register it for the commit-fest so it gets cfbot and other visibility > * modify pgcrypto to use the new AES API (the SHA512 call no longer exists) > * develop TAP tests, though as I mentioned, they will be odd Patch set reverted. This was obviously a bad idea. I also apologize to anyone who was disturbed on Christmas by my activity. -- Bruce Momjian https://momjian.us EnterpriseDB https://enterprisedb.com The usefulness of a cup is in its emptiness, Bruce Lee
