Re: Pg14, pg_dumpall and "password_encryption=true"

On Sun, Jan 17, 2021 at 01:51:35PM +0100, Magnus Hagander wrote:
> 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.

I didn't do a precedent search, but I can't think of an instance of those
programs doing (3). We have things like guessConstraintInheritance() that
make up for lack of information in old versions, but dumpFunc() doesn't mutate
any proconfig setting values. Is there a particular pg_dump behavior you had
in mind?

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

Agreed.

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

This would be interpreting setconfig='{password_encryption=on}' as "opt out of
future password security increases". I expect that will tend not to match the
intent of the person entering the setting. That said, if v14 were already
behaving this way, I wouldn't dislike it enough to complain.