Re: Pg14, pg_dumpall and "password_encryption=true"

On Sat, Jan 16, 2021 at 8:27 AM Noah Misch <noah(at)leadboat(dot)com> wrote:
>
> On Fri, Jan 15, 2021 at 01:35:50PM +0900, Ian Lawrence Barwick wrote:
> > $ tail -3 pg_upgrade_utility.log
> > ALTER ROLE "postgres" SET "password_encryption" TO 'true';
> > psql:pg_upgrade_dump_globals.sql:75: ERROR: invalid value for
> > parameter "password_encryption": "true"
> > HINT: Available values: md5, scram-sha-256.
> >
> > This is a consquence of commit c7eab0e97, which removed support for the
> > legacy
> > values "on"/"true"/"yes"/"1".
>
> Right. This can happen anytime we reduce the domain of a setting having
> GucContext PGC_SU_BACKEND, PGC_BACKEND, PGC_SUSET, or PGC_USERSET.
>
> > I see following options to resolve this issue.
> >
> > 1. Re-add support for a "true" as an alias for "md5".
> > 2. Transparently rewrite "true" to "md5"
> > 3. Have pg_dumpall map "true" to "md5"
> > 4. Add an option to pg_dumpall to specify the target version
>
> I expect rather few databases override this particular setting in
> pg_proc.proconfig or pg_db_role_setting.setconfig. The restore failure has a
> clear error message, which is enough. Each of the above would be overkill.

Option 3 would be the closest to how other things work though,
wuodln't it? Normally, it's the job of pg_dump (or pg_dumpall) to
adapt the dump to the new version of PostgreSQL.

And pg_dump[all] always generates a dump that should reload in the
same version of postgres as the dump program -- not the source
postgresql. Thus doing (4) would mean changing that, and would have to
teach pg_dump[all] about *all* version differences in all directions
-- which would be a huge undertaking. Doing that for just one
individual parameter would be very strange.

In a lot of ways, think (3) seems like the reasonable ting to do.
That's basically what we do when things change in the table creation
commands etc, so it seems like the logical place.

> > 5. Have pg_upgrade emit a warning/hint
>
> If done in a way not specific to this one setting, +1 for this. That is to
> say, do something like the following. Retrieve every pg_proc.proconfig and
> pg_db_role_setting.setconfig value from the old cluster. Invoke the new
> postmaster binary to test acceptance of each value. I'm not generally a fan
> of adding pg_upgrade code to predict whether the dump will fail to restore,
> because such code will never be as good as just trying the restore. That
> said, this checking of GUC acceptance could be self-contained and useful for
> the long term.

Yeah, while I think (3) above would be the better choice in this
particular case, I agree something like that would be useful in a
longer term. It might not always be possible to declare a rule for
mapping values after all..

> > 6. Document this as a backwards-compatibility thing
> > ----------------------------------------------------
> >
> > Add an item in the documentation (release notes, pg_upgrade, pg_dumpall)
> > stating
> > that any occurrences of "password_encryption" which are not valid in Pg14
> > should
> > be updated before performing an upgrade.
>
> The release notes will document the password_encryption change, though they
> probably won't specifically mention the interaction with pg_dump. I would not
> document it elsewhere.

+1.

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/