From: | Peter Eisentraut <peter(dot)eisentraut(at)enterprisedb(dot)com> |
---|---|
To: | Julien Rouhaud <rjuju123(at)gmail(dot)com> |
Cc: | pgsql-hackers <pgsql-hackers(at)postgresql(dot)org> |
Subject: | Re: Database-level collation version tracking |
Date: | 2022-02-07 15:44:24 |
Message-ID: | c04b1477-80f1-34ea-42ab-df171cbd7a50@enterprisedb.com |
Views: | Raw Message | Whole Thread | Download mbox | Resend email |
Thread: | |
Lists: | pgsql-hackers |
On 07.02.22 11:29, Julien Rouhaud wrote:
> - there should be a mention to the need for a catversion bump in the message
> comment
done
> - there is no test
Suggestions where to put it? We don't really have tests for the
collation-level versioning either, do we?
> - it's missing some updates in create_database.sgml, and psql tab completion
> for CREATE DATABASE with the new collation_version defelem.
Added to create_database.sgml, but not to psql. We don't have
completion for the collation option either, since it's only meant to be
used by pg_upgrade, not interactively.
> - that's not really something new with this patch, but should we output the
> collation version info or mismatch info in \l / \dO?
It's a possibility. Perhaps there is a question of performance if we
show it in \l and people have tons of databases and they have to make a
locale call for each one. As you say, it's more an independent feature,
but it's worth looking into.
> + if (!actual_versionstr)
> + ereport(ERROR,
> + (errmsg("database \"%s\" has no actual collation version, but a version was specified",
> + name)));-
>
> this means you can't connect on such a database anymore. The level is probably
> ok for collation version check, but for db isn't that too much?
Right, changed to warning.
> +Oid
> +AlterDatabaseRefreshColl(AlterDatabaseRefreshCollStmt *stmt)
> +{
> + Relation rel;
> + Oid dboid;
> + HeapTuple tup;
> + Datum datum;
> + bool isnull;
> + char *oldversion;
> + char *newversion;
> +
> + rel = table_open(DatabaseRelationId, RowExclusiveLock);
> + dboid = get_database_oid(stmt->dbname, false);
> +
> + if (!pg_database_ownercheck(dboid, GetUserId()))
> + aclcheck_error(ACLCHECK_NOT_OWNER, OBJECT_DATABASE,
> + stmt->dbname);
> +
> + tup = SearchSysCacheCopy1(DATABASEOID, ObjectIdGetDatum(dboid));
> + if (!HeapTupleIsValid(tup))
> + elog(ERROR, "cache lookup failed for database %u", dboid);
>
> Is that ok to not obtain a lock on the database when refreshing the collation?
That code takes a RowExclusiveLock on pg_database. Did you have
something else in mind?
> + /*
> + * Check collation version. See similar code in
> + * pg_newlocale_from_collation().
> + */
> + datum = SysCacheGetAttr(DATABASEOID, tup, Anum_pg_database_datcollversion,
> + &isnull);
> + if (!isnull)
> + {
>
> This (and pg_newlocale_from_collation()) reports a problem if a recorded
> collation version is found but there's no reported collation version.
> Shouldn't it also complain if it's the opposite? It's otherwise a backdoor to
> make sure there won't be any check about the version anymore, and while it can
> probably happen if you mess with the catalogs it still doesn't look great.
get_collation_actual_version() always returns either null or not null
for a given installation. So the situation that the stored version is
null and the actual version is not null can only happen as part of a
software upgrade. In that case, all uses of collations after an upgrade
would immediately start complaining about missing versions, which seems
like a bad experience. Users can explicitly opt in to version tracking
by running REFRESH VERSION once.
> + /*
> + * template0 shouldn't have any collation-dependent objects, so unset
> + * the collation version. This avoids warnings when making a new
> + * database from it.
> + */
> + "UPDATE pg_database SET datcollversion = NULL WHERE datname = 'template0';\n\n",
>
> I'm not opposed, but shouldn't there indeed be a warning in case of discrepancy
> in the source database (whether template or not)?
>
> # update pg_database set datcollversion = 'meh' where datname in ('postgres', 'template1');
> UPDATE 2
>
> # create database test1 template postgres;
> CREATE DATABASE
>
> # create database test2 template template1;
> CREATE DATABASE
>
> # \c test2
> WARNING: database "test2" has a collation version mismatch
I don't understand what the complaint is here. It seems to work ok?
Attachment | Content-Type | Size |
---|---|---|
v3-0001-Database-level-collation-version-tracking.patch | text/plain | 27.0 KB |
From | Date | Subject | |
---|---|---|---|
Next Message | Esteban Zimanyi | 2022-02-07 15:52:22 | Re: Storage for multiple variable-length attributes in a single row |
Previous Message | Robert Haas | 2022-02-07 15:35:43 | Re: [PATCH v2] use has_privs_for_role for predefined roles |