Re: found xmin from before relfrozenxid on pg_catalog.pg_authid

Hi,

On 2018-05-29 19:14:51 -0400, Alvaro Herrera wrote:
> I added an Assert(DatabasePath != NULL) to
> RelationCacheInitFilePreInvalidate() and didn't see a single crash when
> running the tests. I thought that adding a "VACUUM FREEZE pg_class"
> in the recovery tests (where there is a standby) ought to do it, but it
> doesn't. What's the deal there?
>
> Here are some proposed changes. Some of these comment edits are WIP :-)

I'm a bit confused by these changes - there seems to be some that look
like a borked diff? And a number of others look pretty unrelated?

> --
> Álvaro Herrera https://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

> diff --git a/src/backend/utils/cache/inval.c b/src/backend/utils/cache/inval.c
> index 0d6100fb08..8a8620943f 100644
> --- a/src/backend/utils/cache/inval.c
> +++ b/src/backend/utils/cache/inval.c
> @@ -872,6 +872,8 @@ ProcessCommittedInvalidationMessages(SharedInvalidationMessage *msgs,
> int nmsgs, bool RelcacheInitFileInval,
> Oid dbid, Oid tsid)
> {
> + Assert(InRecovery);
> +

Idk, seems unrelated.

> if (nmsgs <= 0)
> return;
>
> @@ -884,12 +886,13 @@ ProcessCommittedInvalidationMessages(SharedInvalidationMessage *msgs,
> dbid);
>
> /*
> - * RelationCacheInitFilePreInvalidate, when the invalidation message
> - * is for a specific database, requires DatabasePath to be set, but we
> - * should not use SetDatabasePath during recovery, since it is
> - * intended to be used only once by normal backends. Hence, a quick
> - * hack: set DatabasePath directly then unset after use.
> + * When the invalidation message is for a specific database,
> + * RelationCacheInitFilePreInvalidate requires DatabasePath to be set,
> + * but we're not allowed to use SetDatabasePath during recovery (since
> + * it is intended to be used by normal backends). Hence, a quick hack:
> + * set DatabasePath directly then unset after use.
> */
> + Assert(!DatabasePath); /* don't clobber an existing value */
> if (OidIsValid(dbid))
> DatabasePath = GetDatabasePath(dbid, tsid);

Same.

> diff --git a/src/backend/utils/cache/relcache.c b/src/backend/utils/cache/relcache.c
> index aa4427724d..71b2212cbd 100644
> --- a/src/backend/utils/cache/relcache.c
> +++ b/src/backend/utils/cache/relcache.c
> @@ -1934,6 +1934,11 @@ RelationIdGetRelation(Oid relationId)
> RelationClearRelation(rd, true);
>
> /*
> + * Normal entries are valid by now -- except nailed ones when
> + * loaded before relcache initialization. There isn't enough
> + * infrastructure yet to do pg_class lookups, but we need their
> + * rd_rel entries to be updated, so we let these through.
> + */
> * Normally entries need to be valid here, but before the relcache
> * has been initialized, not enough infrastructure exists to
> * perform pg_class lookups. The structure of such entries doesn't
> @@ -2346,8 +2351,7 @@ RelationClearRelation(Relation relation, bool rebuild)
> RelationCloseSmgr(relation);

This sure looks like it's a syntax error? So I guess you might not have
staged the removal ports of the diff?

> /*
> - * Treat nailed-in system relations separately, they always need to be
> - * accessible, so we can't blow them away.
> + * We cannot blow away nailed-in relations, so treat them especially.
> */

The former seems just as accurate, and is basically just the already
existing comment?

> if (relation->rd_isnailed)
> {
> @@ -5942,7 +5946,8 @@ write_relcache_init_file(bool shared)
> * wrote out was up-to-date.)
> *
> * This mustn't run concurrently with the code that unlinks an init file
> - * and sends SI messages, so grab a serialization lock for the duration.
> + * and sends SI messages (see RelationCacheInitFilePreInvalidate), so grab
> + * a serialization lock for the duration.
> */
> LWLockAcquire(RelCacheInitLock, LW_EXCLUSIVE);

Unrelated?

> @@ -6061,6 +6066,10 @@ RelationHasUnloggedIndex(Relation rel)
> * changed one or more of the relation cache entries that are kept in the
> * local init file.
> *
> + * When DatabasePath is set, both the init file for that database and the
> + * shared (global) init files are to be removed; otherwise only the latter is.
> + * This is useful during recovery (XXX really?)
> + *

I'm confused?

> * To be safe against concurrent inspection or rewriting of the init file,
> * we must take RelCacheInitLock, then remove the old init file, then send
> * the SI messages that include relcache inval for such relations, and then
> @@ -6180,9 +6189,9 @@ unlink_initfile(const char *initfilename, int elevel)
> {
> if (unlink(initfilename) < 0)
> {
> - /* It might not be there, but log any error other than ENOENT */
> + /* It might not be there, but report any error other than ENOENT */
> if (errno != ENOENT)
> - ereport(ERROR,
> + ereport(elevel,
> (errcode_for_file_access(),
> errmsg("could not remove cache file \"%s\": %m",
> initfilename)));

Included the elevel inclusion. Can't parse the difference between log
and report here?

Greetings,

Andres Freund