Re: Database-level collation version tracking
От | Julien Rouhaud |
---|---|
Тема | Re: Database-level collation version tracking |
Дата | |
Msg-id | 20220208125518.f6kwwbx3qohqqzvr@jrouhaud обсуждение исходный текст |
Ответ на | Re: Database-level collation version tracking (Peter Eisentraut <peter.eisentraut@enterprisedb.com>) |
Ответы |
Re: Database-level collation version tracking
(Peter Eisentraut <peter.eisentraut@enterprisedb.com>)
Re: Database-level collation version tracking (Peter Eisentraut <peter.eisentraut@enterprisedb.com>) Re: Database-level collation version tracking (Alvaro Herrera <alvherre@alvh.no-ip.org>) |
Список | pgsql-hackers |
On Tue, Feb 08, 2022 at 12:14:02PM +0100, Peter Eisentraut wrote: > On 07.02.22 17:08, Julien Rouhaud wrote: > > There's so limited testing in collate.* regression tests, so I thought it would > > be ok to add it there. At least some ALTER DATABASE ... REFRESH VERSION would > > be good, similarly to collation-level versioning. > > I don't think you can run ALTER DATABASE from the regression test scripts, > since the database name is not fixed. You'd have to paste the command > together using psql tricks or something. I guess it could be done, but > maybe there is a better solution. We could put it into the createdb test > suite, or write a new TAP test suite somewhere. There is no good precedent > for this. I was thinking using a simple DO command, as there are already some other usage for that for non deterministic things (like TOAST tables). If it's too problematic I'm fine with not having tests for that for now. > > > That code takes a RowExclusiveLock on pg_database. Did you have something > > > else in mind? > > > > But that lock won't prevent a concurrent DROP DATABASE, so it's totally > > expected to hit that cache lookup failed error. There should either be a > > shdepLockAndCheckObject(), or changing the error message to some errmsg("data > > xxx does not exist"). > > I was not familiar with that function. AFAICT, it is only used for database > and role settings (AlterDatabaseSet(), AlterRoleSet()), but not when just > updating the role or database catalog (e.g., AlterRole(), RenameRole(), > RenameDatabase()). So I don't think it is needed here. Maybe I'm missing > something. I'm just saying that without such a lock you can easily trigger the "cache lookup" error, and that's something that's supposed to happen with normal usage I think. So it should be a better message saying that the database has been concurrently dropped, or actually simply does not exist like it's done in AlterDatabaseOwner() for the same pattern: [...] tuple = systable_getnext(scan); if (!HeapTupleIsValid(tuple)) ereport(ERROR, (errcode(ERRCODE_UNDEFINED_DATABASE), errmsg("database \"%s\" does not exist", dbname))); [...] Apart from that I still think that we should check the collation version of the source database when creating a new database. It won't cost much but will give the DBA a chance to recreate the indexes before risking invalid index usage.
В списке pgsql-hackers по дате отправления: